Bug 10865 - Don't show list permissions when adding public lists/sharing lists is not allowed
Summary: Don't show list permissions when adding public lists/sharing lists is not all...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-10 13:14 UTC by Katrin Fischer
Modified: 2017-06-21 12:59 UTC (History)
5 users (show)

See Also:
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:


Attachments
screenshot (16.03 KB, image/png)
2013-09-10 13:14 UTC, Katrin Fischer
Details
Bug 10865 - Don't show list permissions when adding public lists/sharing lists is not allowed (9.88 KB, patch)
2014-02-05 15:22 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10865 - Don't show list permissions when adding public lists/sharing lists is not allowed (9.96 KB, patch)
2014-02-05 16:16 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed (10.51 KB, patch)
2014-02-07 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed (10.57 KB, patch)
2014-02-10 09:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed (10.80 KB, patch)
2014-02-12 10:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10865: (follow-up) Move a li tag to avoid overlapping tags (1.89 KB, patch)
2014-02-12 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 18065: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off (6.17 KB, patch)
2014-02-21 20:55 UTC, Galen Charlton
Details | Diff | Splinter Review
bug 10865: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off (6.18 KB, patch)
2014-02-25 17:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10865 [Follow-up] Add style to warning (3.31 KB, patch)
2014-02-25 18:05 UTC, Owen Leonard
Details | Diff | Splinter Review
bug 10865: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off (6.26 KB, patch)
2014-02-26 15:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10865 [Follow-up] Add style to warning (3.31 KB, patch)
2014-02-26 15:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed (10.86 KB, patch)
2014-03-11 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10865: (follow-up) Move a li tag to avoid overlapping tags (1.94 KB, patch)
2014-03-11 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
bug 10865: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off (6.28 KB, patch)
2014-03-11 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10865 [Follow-up] Add style to warning (3.33 KB, patch)
2014-03-11 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10865 [Follow-up] Add style to warning (84.15 KB, patch)
2014-04-09 16:48 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed (10.93 KB, patch)
2014-04-09 20:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10865: (follow-up) Move a li tag to avoid overlapping tags (2.01 KB, patch)
2014-04-09 20:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] bug 10865: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off (6.36 KB, patch)
2014-04-09 20:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10865 [Follow-up] Add style to warning (84.76 KB, patch)
2014-04-09 20:48 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-09-10 13:14:17 UTC
Created attachment 20955 [details]
screenshot

We can now prevent patrons from adding public lists, but the interface in the OPAC still shows the pulldown with only 'Private' in that case and also permissions that don't make sense as long as user's can't share lists or work on public lists created from the OPAC together.
Comment 1 Owen Leonard 2014-02-05 15:22:58 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2014-02-05 15:26:05 UTC
Thx Owen!
Comment 3 Nicole C. Engard 2014-02-05 16:16:05 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2014-02-06 07:35:23 UTC
Saving a public list on opac now results in setting it to Private.

Failed QA
Comment 5 Owen Leonard 2014-02-06 16:10:07 UTC
(In reply to M. de Rooy from comment #4)
> Saving a public list on opac now results in setting it to Private.

I can't reproduce this error. Can you tell me the steps you're following? Does it only happen in one theme?
Comment 6 Marcel de Rooy 2014-02-07 08:29:31 UTC
Tested in bootstrap.
Enable pref, create new public list in opac.
Disable pref. Edit same list in opac and Save.
Public list is now a private list..
(This is new behavior; tested it too without the patch.)
Comment 7 Owen Leonard 2014-02-07 14:10:50 UTC
(In reply to M. de Rooy from comment #6)
> Enable pref, create new public list in opac.
> Disable pref. Edit same list in opac and Save.

I see the problem, but this presents an interesting question... If you have changed your library policy to disallow creation of public lists should it be possible for the general public to keep a list public?
Comment 8 Owen Leonard 2014-02-07 14:49:54 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2014-02-07 16:09:23 UTC
(In reply to Owen Leonard from comment #7)
> (In reply to M. de Rooy from comment #6)
> > Enable pref, create new public list in opac.
> > Disable pref. Edit same list in opac and Save.
> 
> I see the problem, but this presents an interesting question... If you have
> changed your library policy to disallow creation of public lists should it
> be possible for the general public to keep a list public?

Good point. If you decide to disallow creation, you should probably also think at that moment about what you are doing with the lists made. (In the ideal situation, changing the pref should trigger such an action?) I think it should not depend on if a user edits his public list sometime after that..
So on the balance, I would stick to keeping it in the same category. (I understand that your patch does that now. I will check later.)
Comment 10 Marcel de Rooy 2014-02-07 20:47:49 UTC
Checking this patch, there is still another small issue:
If the preference is disabled, and I view Public lists, there still is a button New list. This will create a new private list. This is somewhat confusing. Could we not better remove the button New list here?
Comment 11 Marcel de Rooy 2014-02-10 09:07:23 UTC
(In reply to M. de Rooy from comment #10)
> Checking this patch, there is still another small issue:
> If the preference is disabled, and I view Public lists, there still is a
> button New list. This will create a new private list. This is somewhat
> confusing. Could we not better remove the button New list here?

Since it always defaults to Private list (also when I click New list coming from Public lists), I would just leave it that way on second thought.
Will sign off now.
Comment 12 Marcel de Rooy 2014-02-10 09:20:24 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-02-12 10:37:34 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-02-12 10:38:01 UTC Comment hidden (obsolete)
Comment 15 Galen Charlton 2014-02-21 20:54:41 UTC
> (In reply to Owen Leonard from comment #7)
> > (In reply to M. de Rooy from comment #6)
> > > Enable pref, create new public list in opac.
> > > Disable pref. Edit same list in opac and Save.
> > 
> > I see the problem, but this presents an interesting question... If you have
> > changed your library policy to disallow creation of public lists should it
> > be possible for the general public to keep a list public?
> 
> Good point. If you decide to disallow creation, you should probably also
> think at that moment about what you are doing with the lists made. (In the
> ideal situation, changing the pref should trigger such an action?) I think
> it should not depend on if a user edits his public list sometime after that..
> So on the balance, I would stick to keeping it in the same category. (I
> understand that your patch does that now. I will check later.)

I found another edge case with the current patch:

- patron creates public list
- OpacAllowPublicListCreation is turned off
- patron has no way to change the list to be private

I'm about to attach a follow-up that fills that gap, with the follow-up, a patron will be able to make there list private.  Per a discussion on #koha, I'm leaving it to Owen to style the warning message that is part of the follow-up.

Setting to in discussion pending review of my follow-up.
Comment 16 Galen Charlton 2014-02-21 20:55:10 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-02-25 17:14:32 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2014-02-25 18:05:07 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2014-02-26 15:10:26 UTC
(In reply to Owen Leonard from comment #18)
> This patch adds a new "hint" class for displaying information relating
> to a form field. On the list edit screen the hint also has an alert
> class to highlight it.

Owen: What is the exact 'procedure' when making changes now to css file or less file? Who compiles less to css? You did probably now..
Comment 20 Marcel de Rooy 2014-02-26 15:24:55 UTC
The text on the page now on bootstrap is so long actually that it wraps to the second line showing up not very nice. This pertains to the existing permissions line too..
Comment 21 Marcel de Rooy 2014-02-26 15:28:25 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-02-26 15:30:47 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-02-26 15:32:33 UTC
Galen: I agree with your patch. But note that imo the design still needs some attention. But since we cannot argue about tastes too much :), anyone is free to get this to Signed off stage..
Comment 24 sandboxes@biblibre.com 2014-03-11 11:10:09 UTC
Patch tested with a sandbox, by Broust <jean-manuel.broust@univ-lyon2.fr>
Comment 25 sandboxes@biblibre.com 2014-03-11 11:11:00 UTC Comment hidden (obsolete)
Comment 26 sandboxes@biblibre.com 2014-03-11 11:11:05 UTC Comment hidden (obsolete)
Comment 27 sandboxes@biblibre.com 2014-03-11 11:11:09 UTC Comment hidden (obsolete)
Comment 28 sandboxes@biblibre.com 2014-03-11 11:11:13 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2014-03-16 18:55:26 UTC
Ok, I started testing this with all 4 patches applied. But I am confused - OpacAllowSharingPrivateLists is set to "Don't allow", but I can still see the additional options that refer to this feature on creating a new list from the patron account (bootstrap/prog)?

Options I talk about:
Allow/Don't allow
   anyone to remove his own contributed entries.
   anyone to remove other contributed entries. 

URL was:
   http://localhost/cgi-bin/koha/opac-shelves.pl?shelves=1

Only when I additionally turn off OpacAllowPublicListCreation the options disappear.
Comment 30 Marcel de Rooy 2014-03-17 07:51:05 UTC
(In reply to Katrin Fischer from comment #29)
> Ok, I started testing this with all 4 patches applied. But I am confused -
> OpacAllowSharingPrivateLists is set to "Don't allow", but I can still see
> the additional options that refer to this feature on creating a new list
> from the patron account (bootstrap/prog)?
> 
> Options I talk about:
> Allow/Don't allow
>    anyone to remove his own contributed entries.
>    anyone to remove other contributed entries. 

These options do not specifically refer to a shared list. They apply just as well to a public list.
Back to SO.
Comment 31 Katrin Fischer 2014-03-18 07:32:59 UTC
Hi Marcel,

I tend to disagree here and had a library reporting those as confusing. 

If you don't allow sharing, it would be a bit scary to me to be offered those options, leaving me wondering who else can see and change my list and if I misunderstood the feature offered by the library.

>    anyone to remove his own contributed entries.
>    anyone to remove other contributed entries.
Comment 32 Katrin Fischer 2014-03-18 07:35:39 UTC
Hm ok, maybe I start to understand. There used to be 3 types of lists in the very past: private, public and open. I think we don't have open as an option in the pull down anymore... but you would be able to mimick the former behaviour with the new options? Maybe it would be clearer if they only showed up, once you have selected your list to be 'public'? Or the description could be changed/amended.
Comment 33 Marcel de Rooy 2014-03-19 12:27:25 UTC
(In reply to Katrin Fischer from comment #32)
> Hm ok, maybe I start to understand. There used to be 3 types of lists in the
> very past: private, public and open. I think we don't have open as an option
> in the pull down anymore... but you would be able to mimick the former
> behaviour with the new options? Maybe it would be clearer if they only
> showed up, once you have selected your list to be 'public'? Or the
> description could be changed/amended.

Hi Katrin.
Actually, this discussion should be held on report 7310, not here. This report is just a minor detail.
We now have private lists and public lists. With the permissions you can make your list "open" (allowing more flexibility than in the past, with less bugs :).
Note that the three permissions have been constructed in this way after discussions on the dev mailing list (and pushed in 3.8).

If the list is strictly private, the only useful perm is delete your own entries. You could protect yourself from incidentally deleting an item by disallowing that.

But note that if you would start sharing lists [new feature on report 9032] or already if a list changed from public to private etc., things may become more difficult. Still, the perms will just work as they advertise: The owner can always add. Somebody else can add if permitted (but only if he has access). Deleting can be controlled similarly. Due to such exceptions, I would not recommend hiding the three possible perms for private lists; you are not sure if all items on the list have been added by the owner.

Another thought: If you would change public to private, you could of course move all items to the owner of the list.. (New report ? )
Comment 34 Katrin Fischer 2014-04-07 21:48:07 UTC
There is a conflict in opac.css now - I am not sure how this needs to be resolved now with less - Owen, could you take a look please?
Comment 35 Owen Leonard 2014-04-09 16:48:05 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2014-04-09 20:47:25 UTC
Created attachment 26934 [details] [review]
[PASSED QA] Bug 10865 [Revised] Don't show list permissions when adding public lists/sharing lists is not allowed

If patron creation of public lists is disallowed by the
OpacAllowPublicListCreation system preference the "category" option
should be hidden altogether instead of showing a <select> with "private"
as the only option. This patch hides category and permissions controls
when OpacAllowPublicListCreation is set to "don't allow."

To test you must have the virtualshelves system preference enabled.
Apply the patch and log into the OPAC. Test:

- With OpacAllowPublicListCreation enabled, create a new list. You
  should see options for setting category and permissions. Saving the
  new list should complete correctly and save the right settings.
- With OpacAllowPublicListCreation enabled, edit an existing list. You
  should see the same options and saving your changes should work
  correctly.
- With OpacAllowPublicListCreation disabled, create a new list. You
  should only see fields for title and sort. Saving this list should
  complete correctly and save the right settings.
- With OpacAllowPublicListCreation disabled, edit an existing list. You
  should be able to edit only title and sort settings. Saving your
  changes should work correctly.

Repeat your tests for both prog and bootstrap themes.

Revision: Existing public lists can be edited and retain their public
status even if OpacAllowPublicListCreation has since been disabled. This
preserves the behavior previous to this patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch fixes a big ergonomic issue.
Note: to me, the "New list" action should be outside the tabs.
It is confusing to have a "new list" into the public lists tab when it
is not possible to create new public lists.

Signed-off-by: Broust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 37 Katrin Fischer 2014-04-09 20:47:41 UTC
Created attachment 26935 [details] [review]
[PASSED QA] Bug 10865: (follow-up) Move a li tag to avoid overlapping tags

With this patch, a li tag is empty (only using the bootstrap theme) if
the pref is disabled.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Broust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 38 Katrin Fischer 2014-04-09 20:47:54 UTC
Created attachment 26936 [details] [review]
[PASSED QA] bug 10865: (follow-up) allow patrons to make their public lists private when OpacAllowPublicListCreation is off

This patch ensures that patrons continue to have the ability to make
their public lists private for any public lists they control that were
created before the library turned the OpacAllowPublicListCreation
system preference off.

To test:

[1] Ensure OpacAllowPublicListCreation is on.
[2] As a patron, create a public list in the OPAC.  Also, create
    a private list.
[3] Turn OpacAllowPublicListCreation off.
[4] Back in the OPAC, verify that the public list can be edited
    and that there are drop-downs for category and permissions.
    Also verify that there is a warning that the patron cannot
    change it back if they convert a public list to private.
[5] Edit the private list created in step 2.  Verify that the
    category and permissions drop-downs are not displayed.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

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

Signed-off-by: Broust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 39 Katrin Fischer 2014-04-09 20:48:27 UTC
Created attachment 26937 [details] [review]
[PASSED QA] Bug 10865 [Follow-up] Add style to warning

This patch adds a new "hint" class for displaying information relating
to a form field. On the list edit screen the hint also has an alert
class to highlight it.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script. Works as advertised.
Tested with Bootstrap and prog theme. Some notes:

- When OpacAllowPublicListCreation is turned off, the permissions
  don't show.
- When OpacAllowPublicListCreation is turned off, we could also hide
  the Category pull down in the [new list] pop up, as there is only
  Private left as an option.
- Maybe we should move the new list link outside of the tabs?
  When OpacAllowPublicListCreation is turned off, but public lists
  exist, the link 'new list' will still show on the public list tab,
  but a private list will be created.
Comment 40 Galen Charlton 2014-04-09 22:26:27 UTC
Pushed to master.  Thanks, Owen!
Comment 41 Fridolin SOMERS 2014-08-06 07:45:17 UTC
Pushed to 3.14.x, will be in 3.14.10