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.
Created attachment 34233 [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
Files for other languages will be add later.
Created attachment 34249 [details] [review] [Signed-off] 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 Véron <veron@veron.ch>
*** Bug 12838 has been marked as a duplicate of this bug. ***
I agree with a new permission. What for is this line : + if ( $user > 0 ) { To exclude database user (the one in koha-conf.xml) ?
If not user is logged. This code is used for intranet and opac.
"If no user is logged in"
(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 ;)
(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.
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.?
Saw a Bug XXXXX: Add permission for shelves
(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).
(In reply to M. de Rooy from comment #11) > Saw a Bug XXXXX: Add permission for shelves Maybe bug 9833?
(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.
Created attachment 34483 [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 Véron <veron@veron.ch>
(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.
Created attachment 34485 [details] [review] Bug 13417: Limit the permission to delete It's preferable to limit the permission to delete shelves.
(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.
Test plan updated: 4/ You should be able to edit all public lists is 4/ You should be able to delete all public lists
Created attachment 34743 [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 Véron <veron@veron.ch> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 34744 [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>
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?
Created attachment 34942 [details] [review] Bug 13417: (qa follow-up) Rename the permission to delete_public_lists
Created attachment 34943 [details] [review] Bug 13417: Add the permission for all languages
Created attachment 35083 [details] [review] [PASSED QA] 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>
Created attachment 35084 [details] [review] [PASSED QA] 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>
Created attachment 35085 [details] [review] [PASSED QA] Bug 13417: (qa follow-up) Rename the permission to delete_public_lists Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35086 [details] [review] [PASSED QA] Bug 13417: Add the permission for all languages Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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
(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.
Created attachment 35089 [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
Could you please rebase? There's a conflict with pl-PL files
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>
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>
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>
Created attachment 35765 [details] [review] Bug 13417: Add the permission for all languages Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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
Patches pushed to master. Thanks Jonathan!
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.
(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.
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?
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.
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.
Hm, have added bug 13888 with my thoughts.
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.
(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.
See bug 14529.
*** Bug 5763 has been marked as a duplicate of this bug. ***