Bug 13417 - New permission to allow staff to remove public lists
Summary: New permission to allow staff to remove public lists
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 5763 12838 (view as bug list)
Depends on:
Blocks: 14529
  Show dependency treegraph
 
Reported: 2014-12-09 14:52 UTC by Jonathan Druart
Modified: 2017-03-06 08:33 UTC (History)
10 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
Bug 13417: Allow staff members to manage public lists (3.90 KB, patch)
2014-12-09 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13417: Allow staff members to manage public lists (4.09 KB, patch)
2014-12-09 18:32 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13417: Allow staff members to manage public lists (4.08 KB, patch)
2014-12-17 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Limit the permission to delete (6.22 KB, patch)
2014-12-17 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Allow staff members to manage public lists (4.13 KB, patch)
2014-12-28 05:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13417: Limit the permission to delete (6.72 KB, patch)
2014-12-28 05:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13417: (qa follow-up) Rename the permission to delete_public_lists (3.12 KB, patch)
2015-01-05 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Add the permission for all languages (11.66 KB, patch)
2015-01-05 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 13417: Allow staff members to manage public lists (4.10 KB, patch)
2015-01-09 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13417: Limit the permission to delete (6.74 KB, patch)
2015-01-09 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13417: (qa follow-up) Rename the permission to delete_public_lists (3.19 KB, patch)
2015-01-09 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13417: Add the permission for all languages (11.55 KB, patch)
2015-01-09 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13417: Remove warnings (2.86 KB, patch)
2015-01-09 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Allow staff members to manage public lists (4.10 KB, patch)
2015-02-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Limit the permission to delete (6.78 KB, patch)
2015-02-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: (qa follow-up) Rename the permission to delete_public_lists (3.18 KB, patch)
2015-02-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Add the permission for all languages (11.72 KB, patch)
2015-02-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13417: Remove warnings (2.86 KB, patch)
2015-02-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
permissions (15.04 KB, image/png)
2015-03-20 15:22 UTC, Nicole C. Engard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-12-09 14:52:09 UTC
Currently a public list can only be deleted by its owner.
This means lists can exist infinitely.
This will introduce a new permission for list. With this permission, a staff member will be allow to delete any public lists.
Comment 1 Jonathan Druart 2014-12-09 14:56:35 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-12-09 14:57:36 UTC
Files for other languages will be add later.
Comment 3 Marc Véron 2014-12-09 18:32:44 UTC Comment hidden (obsolete)
Comment 4 Fridolin SOMERS 2014-12-11 12:56:36 UTC
*** Bug 12838 has been marked as a duplicate of this bug. ***
Comment 5 Fridolin SOMERS 2014-12-11 13:01:52 UTC
I agree with a new permission.

What for is this line :
+    if ( $user > 0 ) {

To exclude database user (the one in koha-conf.xml) ?
Comment 6 Jonathan Druart 2014-12-11 13:04:39 UTC
If not user is logged.
This code is used for intranet and opac.
Comment 7 Jonathan Druart 2014-12-11 13:05:22 UTC
"If no user is logged in"
Comment 8 Fridolin SOMERS 2014-12-11 13:17:48 UTC
(In reply to Jonathan Druart from comment #6)
> If not user is logged.
> This code is used for intranet and opac.

Ah, OK.
Its because of line 455 in C4/VirtualShelves.pm :
    $user=0 unless $user;

This has an impact : database user as $user=0 so he will not have this permission.
Its not a big deal since this database user should be used only to manage staff users.

Nice job Jonathan ;)
Comment 9 Marcel de Rooy 2014-12-11 14:06:17 UTC
(In reply to Jonathan Druart from comment #0)
> Currently a public list can only be deleted by its owner.
> This means lists can exist infinitely.

Please look for HandleDelBorrower.
When you delete a patron, you delete his lists including the public lists.
Please see bug 11889. It was opened to discuss/find a solution where you do not want to delete all his public lists.
Comment 10 Marcel de Rooy 2014-12-11 14:16:52 UTC
I would favor a permission for allowing staff to manage lists that they do not own. It was discussed on older reports already.

But the solution of this patch is imo little bit too quick.
[1] The change of sub ShelfPossibleAction is not very refined and may have side-effects.
[2] You allow deletion of public lists now, but what if you want to manage a private or shared list from another user? Would it not be better to show a third tab in staff for Other lists (or even another form?) that allows you to search lists, delete, edit, change ownership, etc.?
Comment 11 Marcel de Rooy 2014-12-11 14:19:10 UTC
Saw a Bug XXXXX: Add permission for shelves
Comment 12 Jonathan Druart 2014-12-11 14:25:45 UTC
(In reply to M. de Rooy from comment #9)
> (In reply to Jonathan Druart from comment #0)
> > Currently a public list can only be deleted by its owner.
> > This means lists can exist infinitely.
> 
> Please look for HandleDelBorrower.
> When you delete a patron, you delete his lists including the public lists.
> Please see bug 11889. It was opened to discuss/find a solution where you do
> not want to delete all his public lists.

Ha yes, I forgot that!
But we don't want to remove public lists.
So I could submit a patch to remove private lists only, do you agree with that?

(In reply to M. de Rooy from comment #10)
> I would favor a permission for allowing staff to manage lists that they do
> not own. It was discussed on older reports already.

Code from VirtualShelves is really not easy to read/modify, I preferred not to change it to much.

> But the solution of this patch is imo little bit too quick.
> [1] The change of sub ShelfPossibleAction is not very refined and may have
> side-effects.

I don't think it will introduce side-effects. The change is quite trivial.

> [2] You allow deletion of public lists now, but what if you want to manage a
> private or shared list from another user? Would it not be better to show a
> third tab in staff for Other lists (or even another form?) that allows you
> to search lists, delete, edit, change ownership, etc.?

Discussion appended on a lot of bug reports, and no patch has been pushed yet.
I propose a quick and easy solution to answer a big and real problem for librarians. Even if it is not perfect.

I would not like to add a new tab here, I already have submitted a big change for this page (see bug 13419).
Comment 13 Jonathan Druart 2014-12-11 14:26:08 UTC
(In reply to M. de Rooy from comment #11)
> Saw a Bug XXXXX: Add permission for shelves

Maybe bug 9833?
Comment 14 Marcel de Rooy 2014-12-17 14:36:55 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to M. de Rooy from comment #11)
> > Saw a Bug XXXXX: Add permission for shelves
> 
> Maybe bug 9833?

No the print message in your own patch.
Comment 15 Jonathan Druart 2014-12-17 14:45:06 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2014-12-17 14:46:01 UTC
(In reply to Jonathan Druart from comment #12)
> But we don't want to remove public lists.
> So I could submit a patch to remove private lists only, do you agree with
> that?

> I don't think it will introduce side-effects. The change is quite trivial.
To mention one: you allow now a superlibrarian to delete entries from the list of someone else. The manage permission is not on entry level, but on list level. You can moderate a list name, delete the entire list (for a good reason, i hope), change ownership or so. But why allowing librarians to interfere with the list contents? We respect the user here by not allowing that if he defined it likewise. Easy fix: move your change to the elsif(action eq manage)-part.

> > [2] You allow deletion of public lists now, but what if you want to manage a
> > private or shared list from another user? Would it not be better to show a
> > third tab in staff for Other lists (or even another form?) that allows you
> > to search lists, delete, edit, change ownership, etc.?
> I propose a quick and easy solution to answer a big and real problem for
> librarians. Even if it is not perfect.
> I would not like to add a new tab here, I already have submitted a big
> change for this page (see bug 13419).
Would a new tab really be so much work? And a new form would not interfere with other changes at all. I would prefer a more complete solution.
Comment 17 Jonathan Druart 2014-12-17 15:28:42 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-12-17 15:31:59 UTC
(In reply to M. de Rooy from comment #16)
> (In reply to Jonathan Druart from comment #12)
> > But we don't want to remove public lists.
> > So I could submit a patch to remove private lists only, do you agree with
> > that?
> 
> > I don't think it will introduce side-effects. The change is quite trivial.
> To mention one: you allow now a superlibrarian to delete entries from the
> list of someone else. The manage permission is not on entry level, but on
> list level. You can moderate a list name, delete the entire list (for a good
> reason, i hope), change ownership or so. But why allowing librarians to
> interfere with the list contents? We respect the user here by not allowing
> that if he defined it likewise. Easy fix: move your change to the
> elsif(action eq manage)-part.

The initial need was to delete shelves.
I have just submitted a patch to limit the permission to the shelves deletion.
I hope it matches your requirements.

> > > [2] You allow deletion of public lists now, but what if you want to manage a
> > > private or shared list from another user? Would it not be better to show a
> > > third tab in staff for Other lists (or even another form?) that allows you
> > > to search lists, delete, edit, change ownership, etc.?
> > I propose a quick and easy solution to answer a big and real problem for
> > librarians. Even if it is not perfect.
> > I would not like to add a new tab here, I already have submitted a big
> > change for this page (see bug 13419).
> Would a new tab really be so much work? And a new form would not interfere
> with other changes at all. I would prefer a more complete solution.

I don't think it's inside the scope of this patch.

Switch back to Needs Signoff.
Comment 19 Jonathan Druart 2014-12-17 15:32:29 UTC
Test plan updated:

4/ You should be able to edit all public lists

is

4/ You should be able to delete all public lists
Comment 20 Chris Cormack 2014-12-28 05:15:04 UTC Comment hidden (obsolete)
Comment 21 Chris Cormack 2014-12-28 05:16:05 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-01-02 18:10:41 UTC
Works well! Some QA concerns:
1) Since the name of the feature has changed from "Virtual Shelves" to "Lists" can you change the permissions to "lists" and "manage_lists"?

2) Since this now only controls deletion, should it not be "delete_lists" instead? I think "delete_public_lists" would be most apt, as you cannot delete a patron's private lists.

3) Could you please add the new permissions to each languages permissions sql file?
Comment 23 Jonathan Druart 2015-01-05 09:43:16 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-01-05 09:43:20 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-01-09 14:40:57 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-01-09 14:41:06 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-01-09 14:41:09 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-01-09 14:41:11 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-01-09 14:42:14 UTC
QA Note: One koha-qa.pl failure. AFAICT is not introduced by this patch set.

FAIL	C4/VirtualShelves.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Constant subroutine C4::VirtualShelves::SHELVES_COMBO_MAX redefined
		Constant subroutine C4::VirtualShelves::SHELVES_MGRPAGE_MAX redefined
		Constant subroutine C4::VirtualShelves::SHELVES_MASTHEAD_MAX redefined
		Constant subroutine C4::VirtualShelves::SHARE_INVITATION_EXPIRY_DAYS redefined
		Constant subroutine C4::VirtualShelves::SHELVES_POPUP_MAX redefined
Comment 30 Jonathan Druart 2015-01-09 15:10:51 UTC
(In reply to Kyle M Hall from comment #29)
> QA Note: One koha-qa.pl failure. AFAICT is not introduced by this patch set.

Hum, actually yes, it comes from this patch set.
Comment 31 Jonathan Druart 2015-01-09 15:12:28 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2015-02-09 18:29:59 UTC
Could you please rebase? There's a conflict with pl-PL files
Comment 33 Jonathan Druart 2015-02-10 08:17:17 UTC
Created attachment 35762 [details] [review]
Bug 13417: Allow staff members to manage public lists

Currently a public list can only be deleted by its owner.
This means lists can exist infinitely.
This will introduce a new permission for list. With this permission, a
staff member will be allow to delete any public lists.

Test plan:
1/ Add the manage_shelves permission to a patron.
2/ Login with this patron
3/ Go on the public list view
4/ You should be able to edit all public lists

Followed test plan. Works as expected.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2015-02-10 08:17:25 UTC
Created attachment 35763 [details] [review]
Bug 13417: Limit the permission to delete

It's preferable to limit the permission to delete shelves.

Apply both patches before testing, then follow this test plan

Currently a public list can only be deleted by its owner.
This means lists can exist infinitely.
This will introduce a new permission for list. With this permission, a
staff member will be allow to delete any public lists.

Test plan:
1/ Add the manage_shelves permission to a patron.
2/ Login with this patron
3/ Go on the public list view
4/ You should be able to delete all public lists

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Jonathan Druart 2015-02-10 08:17:31 UTC
Created attachment 35764 [details] [review]
Bug 13417: (qa follow-up) Rename the permission to delete_public_lists

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Jonathan Druart 2015-02-10 08:17:36 UTC
Created attachment 35765 [details] [review]
Bug 13417: Add the permission for all languages

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Jonathan Druart 2015-02-10 08:17:42 UTC
Created attachment 35766 [details] [review]
Bug 13417: Remove warnings

Without this patch
  perl -wc C4/VirtualShelves.pm
failed:
Constant subroutine C4::VirtualShelves::SHELVES_MASTHEAD_MAX redefined
at /usr/share/perl/5.20/constant.pm line 156.
Constant subroutine C4::VirtualShelves::SHELVES_COMBO_MAX redefined at
/usr/share/perl/5.20/constant.pm line 156.
Constant subroutine C4::VirtualShelves::SHELVES_MGRPAGE_MAX redefined at
/usr/share/perl/5.20/constant.pm line 156.
Constant subroutine C4::VirtualShelves::SHELVES_POPUP_MAX redefined at
/usr/share/perl/5.20/constant.pm line 156.
Constant subroutine C4::VirtualShelves::SHARE_INVITATION_EXPIRY_DAYS
redefined at /usr/share/perl/5.20/constant.pm line 156.
Subroutine GetShelves redefined at C4/VirtualShelves.pm line 103.
Subroutine GetAllShelves redefined at C4/VirtualShelves.pm line 159.
Subroutine GetSomeShelfNames redefined at C4/VirtualShelves.pm line 189.
Subroutine GetShelf redefined at C4/VirtualShelves.pm line 230.
Subroutine GetShelfContents redefined at C4/VirtualShelves.pm line 262.
Subroutine AddShelf redefined at C4/VirtualShelves.pm line 315.
Subroutine AddToShelf redefined at C4/VirtualShelves.pm line 353.
Subroutine ModShelf redefined at C4/VirtualShelves.pm line 394.
Subroutine ShelfPossibleAction redefined at C4/VirtualShelves.pm line
454.
Subroutine DelFromShelf redefined at C4/VirtualShelves.pm line 545.
Subroutine DelShelf redefined at C4/VirtualShelves.pm line 589.
Subroutine GetBibliosShelves redefined at C4/VirtualShelves.pm line 603.
Subroutine ShelvesMax redefined at C4/VirtualShelves.pm line 628.
Subroutine HandleDelBorrower redefined at C4/VirtualShelves.pm line 648.
Subroutine AddShare redefined at C4/VirtualShelves.pm line 683.
Subroutine AcceptShare redefined at C4/VirtualShelves.pm line 703.
Subroutine IsSharedList redefined at C4/VirtualShelves.pm line 731.
Subroutine RemoveShare redefined at C4/VirtualShelves.pm line 750.
Subroutine _shelf_count redefined at C4/VirtualShelves.pm line 764.
Subroutine _CheckShelfName redefined at C4/VirtualShelves.pm line 788.
C4/VirtualShelves.pm syntax OK
Comment 38 Tomás Cohen Arazi 2015-02-13 17:14:11 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 39 Nicole C. Engard 2015-03-20 14:41:58 UTC
I'm seeing two new permissions - Lists and then the permission to delete under that.  Does the lists permission do anything? Does it control who can use lists? I'm looking for the manual.
Comment 40 Jonathan Druart 2015-03-20 15:10:41 UTC
(In reply to Nicole C. Engard from comment #39)
> I'm seeing two new permissions - Lists and then the permission to delete
> under that.  Does the lists permission do anything? Does it control who can
> use lists? I'm looking for the manual.

Hi Nicole,
This patchset only introduced 1 'manage_shelves' permission.
If set, the librarian will be able to manage all shelves.
There is no specific permission for deletion.
Comment 41 Nicole C. Engard 2015-03-20 15:22:34 UTC
Created attachment 37051 [details]
permissions

I see two new permissions - and I see that one of the patches here is to rename the delete permission. So my question is why are there 2 permission checkboxes?
Comment 42 Jonathan Druart 2015-03-20 15:37:03 UTC
Sorry yes, it's delete_public_lists (not manage).
And, there is 1 permission in the group of permissions "Lists".
It's the first permission of the "List" module, we can imagine other permissions for this module.
Have a look at the userflags and permissions tables: 1 "flag" can have several permissions.
Comment 43 Katrin Fischer 2015-03-22 07:28:07 UTC
I think it would be more clean to say 'lists' is general list permission with 'manage public lists' and 'remaining list permissions' as the sub permissions - that way it would work like the other module permissions. There is a mismatch there now.
Comment 44 Katrin Fischer 2015-03-22 07:32:47 UTC
Hm, have added bug 13888 with my thoughts.
Comment 45 Pete Edwards 2015-07-13 16:23:13 UTC
This isn't working as I would expect. I just upgraded from 3.18.03 to 3.20.01 and can see the new permissions. I cannot set them since I have superlibrarian permission but that is expected. However, when I go to public lists I still cannot edit or delete them. If I try another user without superlibrarian permission and add the new permission, still nothing changes.
Comment 46 Jonathan Druart 2015-07-13 16:49:44 UTC
(In reply to Pete Edwards from comment #45)
> This isn't working as I would expect. I just upgraded from 3.18.03 to
> 3.20.01 and can see the new permissions. I cannot set them since I have
> superlibrarian permission but that is expected. However, when I go to public
> lists I still cannot edit or delete them. If I try another user without
> superlibrarian permission and add the new permission, still nothing changes.

Hi Pete,
There is a bug, I will provide a patch soon.
Comment 47 Jonathan Druart 2015-07-13 16:56:30 UTC
See bug 14529.
Comment 48 Marcel de Rooy 2017-03-06 08:33:46 UTC
*** Bug 5763 has been marked as a duplicate of this bug. ***