Bug 18980

Summary: Add an explanation when Anyone permission has no actual effect
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: fridolin.somers, jonathan.druart, ldjamison, m.de.rooy, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18834
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18838
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 19255    
Attachments: Bug 18980: Add remark next to "Anyone seeing this list" permission
Bug 18980: Add js to dynamically show/hide the Anyone remark
Bug 18980: Remove js function Check from opac-shelves
Bug 18980: Show distinction between shared and private lists in staff
Bug 18980: Add remark next to "Anyone seeing this list" permission
Bug 18980: Add js to dynamically show/hide the Anyone remark
Bug 18980: Remove js function Check from opac-shelves
Bug 18980: Show distinction between shared and private lists in staff
Bug 18980: Add remark next to "Anyone seeing this list" permission
Bug 18980: Add js to dynamically show/hide the Anyone remark
Bug 18980: Remove js function Check from opac-shelves
Bug 18980: Show distinction between shared and private lists in staff

Description Marcel de Rooy 2017-07-25 13:02:39 UTC
This implements an alternative solution for bug 18834.
Comment 1 Marcel de Rooy 2017-07-25 14:48:53 UTC
Created attachment 65229 [details] [review]
Bug 18980: Add remark next to "Anyone seeing this list" permission

This is a follow-up of report 18228 that simplifies the permissions for
lists. It reapplies the idea behind bug 10865 by adding a remark next to
the Anyone permission if it is not actually effective. A next patch will
make it behave dynamically.

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] Verify that you see the remark now in OPAC and staff.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-07-25 14:48:57 UTC
Created attachment 65230 [details] [review]
Bug 18980: Add js to dynamically show/hide the Anyone remark

If we we move from Private to Public or vice versa, this should reflect
on the Anyone remark from the first patch.
If we go back to a private list without shares, we should show it.

Handled in a js sub AdjustRemark, triggered by onchange of the category
combo or the permission combo.

Test plan:
[1] Edit a private list without shares in OPAC. Toggle category and/or
    permissions. Is the remark shown or hidden accordingly?
    Note: Include a test with OpacAllowPublicListCreation enabled.
[2] Edit a shared list or public list in staff. Toggle category/permissions.
    You should never see the remark.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2017-07-25 14:49:00 UTC
Created attachment 65231 [details] [review]
Bug 18980: 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 4 Marcel de Rooy 2017-07-25 14:49:04 UTC
Created attachment 65232 [details] [review]
Bug 18980: 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 5 Marcel de Rooy 2017-07-25 14:51:31 UTC
I adjusted the patches from 18834 to arrive here. Kept the same division. Note that I removed the signoff from LeeJ on the first two patches. The other two did not really change, but were merely rebased.
Comment 6 Marcel de Rooy 2017-07-25 14:55:09 UTC
LeeJ: Any chance to retest and sign off on the first two ? Would be great!
Comment 7 Marc Véron 2017-07-26 12:47:03 UTC
Created attachment 65273 [details] [review]
Bug 18980: Add remark next to "Anyone seeing this list" permission

This is a follow-up of report 18228 that simplifies the permissions for
lists. It reapplies the idea behind bug 10865 by adding a remark next to
the Anyone permission if it is not actually effective. A next patch will
make it behave dynamically.

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] Verify that you see the remark now in OPAC and staff.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 8 Marc Véron 2017-07-26 12:48:03 UTC
Created attachment 65274 [details] [review]
Bug 18980: Add js to dynamically show/hide the Anyone remark

If we we move from Private to Public or vice versa, this should reflect
on the Anyone remark from the first patch.
If we go back to a private list without shares, we should show it.

Handled in a js sub AdjustRemark, triggered by onchange of the category
combo or the permission combo.

Test plan:
[1] Edit a private list without shares in OPAC. Toggle category and/or
    permissions. Is the remark shown or hidden accordingly?
    Note: Include a test with OpacAllowPublicListCreation enabled.
[2] Edit a shared list or public list in staff. Toggle category/permissions.
    You should never see the remark.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 9 Marc Véron 2017-07-26 12:48:45 UTC
Created attachment 65275 [details] [review]
Bug 18980: 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>

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Marc Véron 2017-07-26 12:49:36 UTC
Created attachment 65276 [details] [review]
Bug 18980: 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>
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 11 Marcel de Rooy 2017-07-26 12:53:13 UTC
(In reply to Marc Véron from comment #10)
> Signed-off-by: Marc Véron <veron@veron.ch>

Thanks, Marc !
Comment 12 Nick Clemens 2017-07-31 14:05:28 UTC
Created attachment 65347 [details] [review]
Bug 18980: Add remark next to "Anyone seeing this list" permission

This is a follow-up of report 18228 that simplifies the permissions for
lists. It reapplies the idea behind bug 10865 by adding a remark next to
the Anyone permission if it is not actually effective. A next patch will
make it behave dynamically.

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] Verify that you see the remark now in OPAC and staff.

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

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2017-07-31 14:05:32 UTC
Created attachment 65348 [details] [review]
Bug 18980: Add js to dynamically show/hide the Anyone remark

If we we move from Private to Public or vice versa, this should reflect
on the Anyone remark from the first patch.
If we go back to a private list without shares, we should show it.

Handled in a js sub AdjustRemark, triggered by onchange of the category
combo or the permission combo.

Test plan:
[1] Edit a private list without shares in OPAC. Toggle category and/or
    permissions. Is the remark shown or hidden accordingly?
    Note: Include a test with OpacAllowPublicListCreation enabled.
[2] Edit a shared list or public list in staff. Toggle category/permissions.
    You should never see the remark.

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

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2017-07-31 14:05:35 UTC
Created attachment 65349 [details] [review]
Bug 18980: 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>

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2017-07-31 14:05:38 UTC
Created attachment 65350 [details] [review]
Bug 18980: 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>
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Jonathan Druart 2017-08-10 16:45:24 UTC
On the staff interface:
"""
    Private list: Is managed by you and can be seen only by you.
    Public list: Can be seen by everybody, but managed only by you.
"""

Should not we update it? If a list is private but shared, the description is wrong.
Comment 17 Jonathan Druart 2017-08-10 16:49:53 UTC
AdjustRemark is a bit awkward, we could show the note only if the shelf is shared, private and changes are allowed by anyone seeing this list.
Not blocker.
Comment 18 Jonathan Druart 2017-08-10 16:53:19 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 19 Marcel de Rooy 2017-09-05 12:42:36 UTC
(In reply to Jonathan Druart from comment #17)
> AdjustRemark is a bit awkward, we could show the note only if the shelf is
> shared, private and changes are allowed by anyone seeing this list.

Hmm. Guess that 20 lines are a bit long to accomplish this, right?
But apart from that, this sub just does what you say it should.
And note it should not only show but it should also hide since it is dynamical and responds to change events on some other controls.
I could compress it a bit, but essentially we need all the checks currently there.
Comment 20 Marcel de Rooy 2017-09-05 12:46:20 UTC
(In reply to Jonathan Druart from comment #16)
> On the staff interface:
> """
>     Private list: Is managed by you and can be seen only by you.
>     Public list: Can be seen by everybody, but managed only by you.
> """
> 
> Should not we update it? If a list is private but shared, the description is
> wrong.

Opened bug 19255
Comment 21 Fridolin Somers 2017-09-29 09:15:32 UTC
Enhancement not pushed to 17.05.x