Bug 7310 - Improving permissions on lists (virtual shelves)
Summary: Improving permissions on lists (virtual shelves)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 2676 5488 5763 5901 6631 6632
Blocks: 4912 7732 7783 28590
  Show dependency treegraph
 
Reported: 2011-12-02 11:12 UTC by Marcel de Rooy
Modified: 2021-06-18 08:30 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (1.72 KB, patch)
2012-01-09 12:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 1]DB revision for Improving list permissions (3.16 KB, patch)
2012-02-16 11:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 2]Changing Koha SQL structure (3.91 KB, patch)
2012-02-16 11:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 3]New prefs on opac tab (1.69 KB, patch)
2012-02-16 11:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 4]Code revisions for improving permissions (81.73 KB, patch)
2012-02-16 11:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 5]Deleting patron (4.79 KB, patch)
2012-02-16 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 6]Updated unit test (2.72 KB, patch)
2012-02-20 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 1][REVISED]DB revision for Improving list permissions (3.15 KB, patch)
2012-02-23 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 2][REVISED]Changing Koha SQL structure (3.91 KB, patch)
2012-02-23 09:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 4][REVISED]Code revisions for improving permissions (81.78 KB, patch)
2012-02-23 09:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 4][REVISED]Code revisions for improving permissions (82.02 KB, patch)
2012-02-23 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 4][REVISED]Code revisions for improving permissions (81.16 KB, patch)
2012-02-23 13:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 5]Deleting patron (4.79 KB, patch)
2012-02-23 13:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH 7]Followup for warnings on somedeleted and nothingdeleted in staff client. (1.55 KB, patch)
2012-02-23 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (3.21 KB, patch)
2012-02-25 21:34 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Improving list permissions: Changing Koha SQL structure (3.98 KB, patch)
2012-02-25 21:34 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Improving lists permissions -- New prefs (1.76 KB, patch)
2012-02-25 21:34 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Code changes for Improving list permissions (81.22 KB, patch)
2012-02-25 21:35 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Improving list permissions: Deleting patron (4.85 KB, patch)
2012-02-25 21:35 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Updated VirtualShelves unit test (2.79 KB, patch)
2012-02-25 21:35 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310 Followup: Warnings for nothingdeleted and somedeleted for staff client. (1.62 KB, patch)
2012-02-25 21:35 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7310: Indentation followup (21.63 KB, patch)
2012-02-27 20:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: Indentation followup (21.46 KB, patch)
2012-02-27 20:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (3.73 KB, patch)
2012-02-28 16:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: Improving list permissions: Changing Koha SQL structure (5.03 KB, patch)
2012-02-28 16:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (3.76 KB, patch)
2012-02-29 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: Code changes for Improving list permissions (81.20 KB, patch)
2012-02-29 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (4.35 KB, patch)
2012-02-29 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: Followup for sysprefs.sql (1.90 KB, patch)
2012-03-01 07:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (17.87 KB, patch)
2012-03-05 08:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: DB revision for Improving list permissions (4.37 KB, patch)
2012-03-05 08:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310: Code changes for Improving list permissions (81.64 KB, patch)
2012-03-05 08:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310 Indentation followup replacing leading tabs with spaces (21.50 KB, patch)
2012-03-05 08:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310 Part 1: DB revision for Improving list permissions (4.36 KB, patch)
2012-03-19 11:55 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 2: Improving Lists permissions new prefs (1.77 KB, patch)
2012-03-19 11:56 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 3: improving list permissions changing Koha SQL structure (5.04 KB, patch)
2012-03-19 11:57 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 4: Code changes [main part] (81.65 KB, patch)
2012-03-19 11:58 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 5: deleting patron (4.87 KB, patch)
2012-03-19 11:59 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 6: VirtualShelves unit test (2.80 KB, patch)
2012-03-19 12:00 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 7: followup for sysprefs (1.99 KB, patch)
2012-03-19 12:01 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 8: Indentation cleanup (21.52 KB, patch)
2012-03-19 12:02 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 9: followup for warnings in nothingdeleted and somedeleted in staff client (1.63 KB, patch)
2012-03-19 12:03 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7310 Part 1: DB revision for Improving list permissions (4.48 KB, patch)
2012-03-21 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310 Part 4: Code changes [main part] (82.92 KB, patch)
2012-03-21 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7310 Part 7: followup for sysprefs (1.99 KB, patch)
2012-03-21 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Screen shot for new permissions (82.37 KB, image/jpeg)
2012-03-22 08:35 UTC, Marcel de Rooy
Details
Screenshot for new permissions (82.54 KB, image/jpeg)
2012-03-22 13:58 UTC, Marcel de Rooy
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2011-12-02 11:12:57 UTC
Following up on report 7281 (hide lists from OPAC) I am sending the following question to the ml in order to improve lists management:

1 Who should be allowed to delete a public [closed] list (category 2 only!) or add/remove items from that list? (Note that removing all items from a list is more or less the same as removing the list at once.)
My answer would be: the owner should only be allowed to do that in the OPAC. A user in the staff client may do it too.
[Currently, any opac login can delete the list. This is a bug!]

2 Who should be allowed to delete an public open list (category 3 only!) or add/remove items from that list?
My answer would be: any login from OPAC or staff. It should be made clear that creating such a list is really at your own risk ;) Currently, a user in the opac cannot create an open list. (Is that a bug?)

3 Proposed revisions to lists management:
If we solve points 1 and 2, management of lists comes down to all or nothing. You can do everything with an open list, but if you are not owning a closed list, you can't do anything. Would it be useful to have something in between? Like deleting your own added items from a closed list (we could save borrowernumber in virtualshelfcontents)?? Or give the owner of an open list the choice between full access for other logins, adding items only or deleting own added items?
What do you think?
Comment 1 Ian Walls 2011-12-02 14:28:58 UTC
There are a great many other reports in here that also deal with List permissions.  I think a single, unified discussion would be beneficial.  Adding links; can mark as dupes later, if we like.
Comment 2 Marcel de Rooy 2011-12-05 10:42:33 UTC
New proposal to the list:

1  Do not allow to create a public list in the OPAC. (Only private lists.)  Public lists are only created by staff users in the staff client. 
2  Add three permission options to any list: a) Allow adding entries b) Allow deleting your own entries (that you added)  and c) Allow deleting entries that someone else added.
   This makes the distinction between public list and open list no longer needed and adds some refinement in lists management. 
   Only the owner of the list can change these permissions.
3  Add a new (opac) feature to private lists: Share a list (with another patron).
   Let the user share access to a list by Koha sending an email with a URL including some (temporary) invitation key. When the invited patron clicks that URL (when logged in) he gains access (in accordance with the described permission options for that specific list).
   The invited patron can always 'delete' the shared list, i.e. delete the share.
   The owner can 'unshare' the list and remove all shares for that list.
4  With respect to user privacy, a feature may be added in staff client to moderate shared list names. 
5  Possibly, libraries do not want patrons sharing lists. So the option could be disabled with a preference. In that case points 2 and 3 still apply.
Comment 3 Nicole C. Engard 2011-12-05 14:29:34 UTC
(In reply to comment #2)
> New proposal to the list:
> 
> 1  Do not allow to create a public list in the OPAC. (Only private lists.) 
> Public lists are only created by staff users in the staff client. 


I think this should be a preference that the librarians can choose from the following options :

   1. Allow anyone to create public lists without moderation
   2. Allow patrons to create public lists with moderation
   3. Allow only staff (with permission) to create public lists

> 2  Add three permission options to any list: a) Allow adding entries b) Allow
> deleting your own entries (that you added)  and c) Allow deleting entries that
> someone else added.
>    This makes the distinction between public list and open list no longer
> needed and adds some refinement in lists management. 
>    Only the owner of the list can change these permissions.

In addition I'd say we need general staff list permissions which we don't have right now.

> 3  Add a new (opac) feature to private lists: Share a list (with another
> patron).
>    Let the user share access to a list by Koha sending an email with a URL
> including some (temporary) invitation key. When the invited patron clicks that
> URL (when logged in) he gains access (in accordance with the described
> permission options for that specific list).
>    The invited patron can always 'delete' the shared list, i.e. delete the
> share.
>    The owner can 'unshare' the list and remove all shares for that list.

I like!

> 4  With respect to user privacy, a feature may be added in staff client to
> moderate shared list names. 
> 5  Possibly, libraries do not want patrons sharing lists. So the option could
> be disabled with a preference. In that case points 2 and 3 still apply.

Yes, preferences to turn things off are always good.
Comment 4 Marcel de Rooy 2011-12-08 12:42:30 UTC
I have created a page on the wiki for the results of this discussion on the mailing list about improving list permissions.
See: http://wiki.koha-community.org/wiki/List_permissions
Will add a section there to describe the patches implementing this change.
Comment 5 Marcel de Rooy 2012-01-09 12:19:59 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2012-01-23 10:47:02 UTC
Comment on attachment 7092 [details] [review]
Patch

Moved this patch to report 7459. It does not (really) belong here..
Comment 7 Marcel de Rooy 2012-02-16 11:39:38 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2012-02-16 11:41:34 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2012-02-16 11:42:47 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2012-02-16 11:49:23 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2012-02-16 11:51:41 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2012-02-16 11:53:28 UTC
Submitted these patches yet, will add some more documentation, test plan via wiki soon.
Comment 13 Marcel de Rooy 2012-02-20 08:46:01 UTC
TEST PLAN for first five patches (from wiki):

Login with user 2 in staff. Create user 1 for opac (to be deleted at the end). Enable OpacAllowPublicListCreation.
 
Login with user 1 in opac. Add a private list V1 via shelves screen. Add a private list V2 via search results, Add to combo. Add some items to both lists. Add a public list P1 via shelves, P2 via search results (Save to another list). Add some items.
 
Login with user 2 in staff. Goto Lists. Check P1 and P2. Try to rename or delete (should be impossible). Add some items via catalog detail and search results (should be possible). Add private V3 via shelves, private V4 via search results, public P3. Add some items.
 
Login with user 1 in opac. Try to rename or delete P3 (should not be possible). Add items to P3 (should be possible). Try to delete an item from P3 that you did not add (user 2 did): should not be possible. Delete an item that you added yourself (should be possible). Delete list V1.
 
Login with user 2 in staff. Delete list P3. Try to delete all items from P2 (only the ones you added should be deleted). Delete patron user 1. Check if you still have P1 and P2. Check items on P2. 

Login with user 2 in opac. Check again if you still have P1 and P2. Check items on P2.
 
Login with user 2 in staff. Disable OpacAllowPublicListCreation. 

Login with user 2 in opac. Try to add a public list via shelves, search results (AddTo combo: New List) and search results (Save to another list). Should not be possible.
 
Logout with user 2 in opac. You are anonymous user now. Check lists P1 and P2 (you should see items). Try to delete a list or delete an entry from a list. (Should not be possible.) Try to create a list. Try to add an item to P1 via search results, opac detail (should not be possible).
Comment 14 Marcel de Rooy 2012-02-20 10:33:26 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2012-02-22 15:15:28 UTC
(In reply to comment #13)
> Login with user 2 in staff. Goto Lists. Check P1 and P2. Try to rename or
> delete (should be impossible). Add some items via catalog detail and search
> results (should be possible).
...
> Add items to P3 (should be possible).

So if OpacAllowPublicListCreation is ON, creating a public list means creating a list which is not only viewable by the everyone but can be added to by everyone? If I have that right that seems like a big change from how it has worked in the past.

> (only the ones you added should be deleted). Delete patron user 1. Check if you
> still have P1 and P2. Check items on P2.

What should happen here? Should deleting the patron delete the list? I see that after deleting patron user 1's private shelves are still in the database. Shouldn't they be deleted? If they have public shelves, what should happen to them? Right now I see patron user 1's public shelves in the staff interface but I can't edit or delete them.
Comment 16 Marcel de Rooy 2012-02-22 16:08:54 UTC
Hi Owen,
Thx for testing!
First question: Yes, if you create a public list now, anyone can indeed add an item. Should we change that back to not allowing to create?
Note that current functionality does not allow me to add an entry, but does allow me to remove an entry (from someone else) from a public list. In the new situation that cannot be done anymore on the contrary.
The defaults when creating a public list can easily be changed however. Or do you think that it is better to rightaway introduce the three new allow flags when creating a list? 

Second question: The private shelves (which do not have shares yet) should indeed be deleted. My (repeated) testing confirmed that. (Could you check that?)
The public shelves are not deleted but the owner is set to null. This keeps them visible. On the wiki page I mentioned an option for a followup to let a staff user change ownership for such lists. But it would not be a must to do that.
Comment 17 Owen Leonard 2012-02-22 16:37:52 UTC
(In reply to comment #16)
> First question: Yes, if you create a public list now, anyone can indeed add an
> item. Should we change that back to not allowing to create?

I think it is contrary to current expectations for others to be able to add to a public list.

> Note that current functionality does not allow me to add an entry, but does
> allow me to remove an entry (from someone else) from a public list.

I do not find this to be the case in current master. I see no option for deleting items from a public list when I'm logged in as someone other than the list creator.

> Second question: The private shelves (which do not have shares yet) should
> indeed be deleted. My (repeated) testing confirmed that. (Could you check
> that?)

I will test again.

> The public shelves are not deleted but the owner is set to null. This keeps
> them visible. On the wiki page I mentioned an option for a followup to let a
> staff user change ownership for such lists. But it would not be a must to do
> that.

Why would it not be a must to do that? If the list has no owner there's no way to delete it, correct?
Comment 18 Marcel de Rooy 2012-02-23 09:25:38 UTC
(In reply to comment #17)
> I think it is contrary to current expectations for others to be able to add to
> a public list.
Will send an adjusted patch.

> I do not find this to be the case in current master. I see no option for
> deleting items from a public list when I'm logged in as someone other than the
> list creator.
Tested in current master: Let user 1 create a public list and add an item. Login as user 2. Goto opac-shelves by clicking Lists button. Select the newly created public list. You can select the item and click Remove selected items. List is empty.

> Why would it not be a must to do that? If the list has no owner there's no way
> to delete it, correct?
I mean this: As long as you do not have a need to rename or delete, the public list can remain as it is. (There is no burden for the librarians to immediately examine public lists without owner.) Depending on the permissions set, people can even add or delete items. I agree that if you want to delete it, you must get ownership. Will be handled in a followup.
Comment 19 Marcel de Rooy 2012-02-23 09:54:48 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2012-02-23 09:56:42 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2012-02-23 09:58:15 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2012-02-23 11:15:51 UTC
(In reply to comment #15)
> Should deleting the patron delete the list? I see that
> after deleting patron user 1's private shelves are still in the database.
Owen, maybe you did not apply patch 5 (deleting patron) when you tested this one? Happened to me now too when testing patch 4 again..
Comment 23 Marcel de Rooy 2012-02-23 11:27:57 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2012-02-23 12:04:33 UTC
Please wait: Found something. Another version of patch 4 is coming..
Comment 25 Marcel de Rooy 2012-02-23 13:20:02 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2012-02-23 13:21:30 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2012-02-23 13:24:38 UTC
ADJUSTED TEST PLAN (also on wiki):

Login with user 2 in staff. Create user 1 for opac (to be deleted at the end). Enable OpacAllowPublicListCreation.
 
Login with user 1 in opac. Add a private list V1 via shelves screen. Add a private list V2 via search results, Add to combo. Add some items to both lists. Add a public list P1 via shelves, P2 via search results (Save to another list). Add some items.
 
Login with user 2 in staff. Goto Lists. Check P1 and P2. Try to rename or delete (should be impossible). Add some items via catalog detail and search results (should NOT be possible). Add private V3 via shelves, private V4 via search results, public P3. Add some items.
 
Login with user 1 in opac. Try to rename or delete P3 (should not be possible). Add items to P3 (should NOT be possible). Delete list V1.
 
Login with user 2 in staff. Delete list P3. Try to delete all items from P2 (should not be possible). Delete patron user 1. Check if you still have P1 and P2. Check items on P2.
 
Login with user 2 in opac. Check again if you still have P1 and P2. Check items on P2.
 
Login with user 2 in staff. Disable OpacAllowPublicListCreation. 

Login with user 2 in opac. Try to add a public list via shelves, search results (AddTo combo: New List) and search results (Save to another list). Should not be possible.
 
Logout with user 2 in opac. Check lists P1 and P2 (you should see items). Try to delete a list or delete an entry from a list. (Should not be possible.) Try to create a list. Try to add an item to P1 via search results, opac detail (should not be possible).
Comment 28 Marcel de Rooy 2012-02-23 13:30:46 UTC
Wiki page http://wiki.koha-community.org/wiki/List_permissions also lists a few minor issues to be taken care of in a followup.
Comment 29 Marcel de Rooy 2012-02-23 13:45:52 UTC Comment hidden (obsolete)
Comment 30 Jared Camins-Esakov 2012-02-25 21:34:02 UTC Comment hidden (obsolete)
Comment 31 Jared Camins-Esakov 2012-02-25 21:34:21 UTC Comment hidden (obsolete)
Comment 32 Jared Camins-Esakov 2012-02-25 21:34:35 UTC Comment hidden (obsolete)
Comment 33 Jared Camins-Esakov 2012-02-25 21:35:13 UTC Comment hidden (obsolete)
Comment 34 Jared Camins-Esakov 2012-02-25 21:35:25 UTC Comment hidden (obsolete)
Comment 35 Jared Camins-Esakov 2012-02-25 21:35:40 UTC Comment hidden (obsolete)
Comment 36 Jared Camins-Esakov 2012-02-25 21:35:54 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2012-02-27 07:51:06 UTC
jcamins: thx for testing and signing off!
Comment 38 Paul Poulain 2012-02-27 16:51:35 UTC
QA comment:
1- am I wrong if I say that the most important new features are not here yet (share a list, temporary share via a temporary key)
2- Am I wrong if I say that the user can't (at least for instance) set allow_add, allow_delete_own, allow_delete_other themselves
3- you have added many sub to opac-addbybiblionumber.pl, without any pod doc, please add at least a few lines for each of them
4- please confirm those sub in opac-addbybiblionumber.pl are not worth being moved to C4::
5- there are many tab, please change for 4 spaces (PERL6 http://wiki.koha-community.org/wiki/Coding_Guidelines)
6- the name "GetSomeShelfNames" sound strange and unusal. What does the "Some" mean ? shouldn't it be GetShelfNamesBy... instead ?
7- borrower deletion: I agree that deletion of a borrower should not result always in shelved deletion. BUT there should be fallback with FK, just in case borrowers are deleted from mySQL, not through Koha. I mean: the HandleDelBorrower should not prevent us from having a FK


* do you have a timeline for #1 and #2 ?
* marking failed QA for 3 and 5
* please answer to 4, 6 and 7, maybe with a follow-up, maybe with a comment
Comment 39 Marcel de Rooy 2012-02-27 18:27:35 UTC
(In reply to comment #38)
> 1- am I wrong if I say that the most important new features are not here yet
> (share a list, temporary share via a temporary key)
> 2- Am I wrong if I say that the user can't (at least for instance) set
> allow_add, allow_delete_own, allow_delete_other themselves
On the wiki I did present a gradual implementation of these features. First laying the foundation and staying close to current functionality, while at the same time already resolving a lot(!) of minor issues with lists.
I do not have an exact timeline because I do not know when (and even if) these patches are coming through. As soon as they are, the first followup will deal with exposing the permissions. This will be in one or two weeks. The share feature will follow after that (perhaps one or two months).

> 3- you have added many sub to opac-addbybiblionumber.pl, without any pod doc,
> please add at least a few lines for each of them
> 4- please confirm those sub in opac-addbybiblionumber.pl are not worth being
> moved to C4::
The code in both scripts was very hard to read and has been refactored with this patch. IMO they are just local subroutines, no candidates for a module. In that view I did not include pod doc. If you think that it would be beneficial, I will make a follow-up for that.
Comment 40 Marcel de Rooy 2012-02-27 18:45:08 UTC
(In reply to comment #38)
> 5- there are many tab, please change for 4 spaces (PERL6
> http://wiki.koha-community.org/wiki/Coding_Guidelines)
Oops, this is not my greatest specialty. Can we run something generic on the files where many lines were changed? Perhaps also in a specific commit that only does that kind of thing? 

> 6- the name "GetSomeShelfNames" sound strange and unusal. What does the "Some"
> mean ? shouldn't it be GetShelfNamesBy... instead ?
I called it Some, because that is what the routine does (and what Koha already did). The routine does not get all columns, but only returns names and numbers for some shelves (10 to 20). But I would be happy to send a followup for a better name, agreeing that it sounds unusual. Do you have a suggestion? 

> 7- borrower deletion: I agree that deletion of a borrower should not result
> always in shelved deletion. BUT there should be fallback with FK, just in case
> borrowers are deleted from mySQL, not through Koha. I mean: the
> HandleDelBorrower should not prevent us from having a FK
There are already some FKs here with a cascaded delete and that works fine. If you delete e.g. a shelf, all biblios and all shares will be automatically purged that way. (I discovered now that the lines deleting them in HandleDelBorrower are not needed.) The advantage of not having the cascade on owner in virtualshelves itself helps us to retain public and shared lists. The routine purges unused shared lists. A followup in the form of an admin job could purge public lists without owner, if a library really wants to do so.
In summary, I think that the situation improves compared to current Koha.
Comment 41 Chris Cormack 2012-02-27 19:19:33 UTC
(In reply to comment #40)
> (In reply to comment #38)
> > 5- there are many tab, please change for 4 spaces (PERL6
> > http://wiki.koha-community.org/wiki/Coding_Guidelines)
> Oops, this is not my greatest specialty. Can we run something generic on the
> files where many lines were changed? Perhaps also in a specific commit that
> only does that kind of thing? 
> 
Do you use vim? If so check out the vim page on the koha wiki you can set it up so you can select text and perltidy it that would fix this.
Comment 42 Marcel de Rooy 2012-02-27 20:43:18 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2012-02-27 20:44:01 UTC
Wrong version. Please wait..
Comment 44 Marcel de Rooy 2012-02-27 20:45:58 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2012-02-28 14:27:22 UTC
Delayed additional comment on point 7 from Paul: I got your point now. I will add a FK to owner in virtualshelves without compromising the whole retaining public lists-stuff as described. Please wait one moment ..
Comment 46 Marcel de Rooy 2012-02-28 16:10:02 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2012-02-28 16:11:56 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2012-02-28 17:05:51 UTC
While testing it again, now stumble over:
[Tue Feb 28 17:54:43 2012] [error] [client 82.173.53.17] [Tue Feb 28 17:54:43 2012] addbybiblionumber.pl: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`kohatest/virtualshelfcontents`, CONSTRAINT `shelfcontents_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE) at /usr/share/koha/testclone/C4/VirtualShelves.pm line 366., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/virtualshelves/addbybiblionumber.pl?newshelf=1&biblionumber=198700&biblionumber=120300&biblionumber=120500

Will check ASAP..
Comment 49 Marcel de Rooy 2012-02-28 17:12:33 UTC
(In reply to comment #48)
> While testing it again, now stumble over:
> [Tue Feb 28 17:54:43 2012] [error] [client 82.173.53.17] [Tue Feb 28 17:54:43
> 2012] addbybiblionumber.pl: DBD::mysql::st execute failed: Cannot add or update
> a child row: a foreign key constraint fails (`kohatest/virtualshelfcontents`,
> CONSTRAINT `shelfcontents_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES
> `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE) at
> /usr/share/koha/testclone/C4/VirtualShelves.pm line 366., referer:
> http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/virtualshelves/addbybiblionumber.pl?newshelf=1&biblionumber=198700&biblionumber=120300&biblionumber=120500
> 
> Will check ASAP..

Well. Just confirms that the FK works. I was adding a biblio from an old Zebra index that did not exist anymore ;)
Comment 50 Marcel de Rooy 2012-02-28 17:19:35 UTC
Still checking another issue, please keep this report on HOLD now.
Comment 51 Marcel de Rooy 2012-02-29 12:39:12 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2012-02-29 12:41:27 UTC Comment hidden (obsolete)
Comment 53 Marcel de Rooy 2012-02-29 12:44:16 UTC
This should do it for now. Further documentation followups coming later. Note that the Deleting patron patch should follow the Code changes patch. Indentation patch should be the last one.

Setting status back to Signed off now.
Comment 54 Marcel de Rooy 2012-02-29 13:20:39 UTC
This report is driving me crazy: I will send another dbrev update in order to make sure that the new FK will not block updating. If there are lists with a deleted owner, they should be set to null before adding the constraint.
Updated patch dbrev forthcoming..
Comment 55 Marcel de Rooy 2012-02-29 14:24:21 UTC Comment hidden (obsolete)
Comment 56 Marcel de Rooy 2012-03-01 07:31:18 UTC Comment hidden (obsolete)
Comment 57 Marcel de Rooy 2012-03-05 08:03:17 UTC Comment hidden (obsolete)
Comment 58 Marcel de Rooy 2012-03-05 08:04:51 UTC
Comment on attachment 8006 [details] [review]
Bug 7310: DB revision for Improving list permissions

Wrong file
Comment 59 Marcel de Rooy 2012-03-05 08:07:01 UTC Comment hidden (obsolete)
Comment 60 Marcel de Rooy 2012-03-05 08:08:42 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2012-03-05 08:10:42 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2012-03-05 08:17:24 UTC
Ongoing effort to keep this up-to-date. Note that order of patches is relevant. I just applied in this order:

Applying: Bug 7310: DB revision for Improving list permissions
Applying: Bug 7310: Improving lists permissions -- New prefs
Applying: Bug 7310: Improving list permissions: Changing Koha SQL structure
Applying: Bug 7310: Code changes for Improving list permissions
Applying: Bug 7310: Improving list permissions: Deleting patron
Applying: Bug 7310: Updated VirtualShelves unit test
Applying: Bug 7310: Followup for sysprefs.sql
Applying: Bug 7310 Indentation followup replacing leading tabs with spaces
Applying: Bug 7310 Followup: Warnings for nothingdeleted and somedeleted for staff client.
Comment 63 Ian Walls 2012-03-17 21:17:29 UTC
After a few rebasings for the changes in updatedatabase and sysprefs.sql, I've got these patches applied in the right order and ready to QA.  The bulk of them are very small and easy to read.  They present no problems I can see.

The major patch, Code changes for Improving list permissions, is where most of the magic happens. It affects both C4/Auth and the C4 Virtual shelves modules, as well as several scripts and templates.  I haven't spotted anything problematic yet, but I'm getting a little tired and pressed for time, so I'll need to pick this up tomorrow.  I just wanted to note that this is in progress, and should have a final pass/fail shortly.

Would folks prefer that, if and when the patches pass QA, I reapply them to the report in the correct order, or squash them into a single patch, and obsolete the rest?
Comment 64 Marcel de Rooy 2012-03-19 07:38:22 UTC
(In reply to comment #63)
> I haven't spotted
> anything problematic yet, but I'm getting a little tired and pressed for
> time, so I'll need to pick this up tomorrow.  I just wanted to note that
> this is in progress, and should have a final pass/fail shortly.
Great! Thanks for your QA. Look forward to final pass ;)

> Would folks prefer that, if and when the patches pass QA, I reapply them to
> the report in the correct order, or squash them into a single patch, and
> obsolete the rest?
Would not squash them. If this report just keeps pending a few days longer in the next queue, rebasing a squashed coding changes and indentation could be a nightmare, just to mention the worst example.
Comment 65 Ian Walls 2012-03-19 11:53:13 UTC
Everything in the core patch looks really good.  While the exported subroutines of C4/VirtualShelves.pm were changed greatly, no hanging calls were left in the rest of the system.  The SQL and logic look clean.

One small thing: C4/Auth.pm removed the export of template param "bartotal", but this is still used in one template, opac/opac-results.tt, to display a More Lists option in the dropdown menu.  It doesn't appear that this param gets exported anywhere else, so I believe it will fail to display the More Lists option at any time.  I don't know if this is particularly relevant, and I'm not sure how best to deal with it, but any fix for this can be handled with a followup patch.

Marking as Passed QA.  Will reattach patches in correct order in a moment
Comment 66 Ian Walls 2012-03-19 11:55:20 UTC Comment hidden (obsolete)
Comment 67 Ian Walls 2012-03-19 11:56:17 UTC
Created attachment 8260 [details] [review]
Bug 7310 Part 2: Improving Lists permissions new prefs
Comment 68 Ian Walls 2012-03-19 11:57:27 UTC
Created attachment 8261 [details] [review]
Bug 7310 Part 3: improving list permissions changing Koha SQL structure
Comment 69 Ian Walls 2012-03-19 11:58:49 UTC Comment hidden (obsolete)
Comment 70 Ian Walls 2012-03-19 11:59:35 UTC
Created attachment 8263 [details] [review]
Bug 7310 Part 5: deleting patron
Comment 71 Ian Walls 2012-03-19 12:00:23 UTC
Created attachment 8264 [details] [review]
Bug 7310 Part 6: VirtualShelves unit test
Comment 72 Ian Walls 2012-03-19 12:01:26 UTC Comment hidden (obsolete)
Comment 73 Ian Walls 2012-03-19 12:02:07 UTC
Created attachment 8266 [details] [review]
Bug 7310 Part 8: Indentation cleanup
Comment 74 Ian Walls 2012-03-19 12:03:01 UTC
Created attachment 8267 [details] [review]
Bug 7310 Part 9: followup for warnings in nothingdeleted and somedeleted in staff client
Comment 75 Marcel de Rooy 2012-03-19 12:51:18 UTC
Will send a followup under a new report number in order to not disturb further processsing of this set of patches.
See bug 7732.
Comment 76 Paul Poulain 2012-03-21 10:29:13 UTC
I'm really sorry Marcel, but patch 4 [main part] does not apply anymore. There's a large conflict on Auth.pm that I can't solve easily. Could you rebase please ?
Comment 77 Marcel de Rooy 2012-03-21 12:12:53 UTC
Will rebase again.. What is the easiest way to get the patches again in the right order without git bz?
Comment 78 Paul Poulain 2012-03-21 13:07:02 UTC
(In reply to comment #77)
> Will rebase again.. What is the easiest way to get the patches again in the
> right order without git bz?

I don't think there's an easy option. Why don't you want to go the git-bz way ?

Otherwise, as the patch already passed QA, i can take care of ordering the patches correctly when pushing.
Comment 79 Marcel de Rooy 2012-03-21 13:09:14 UTC
Will install git-bz on a new dev server soon.
Comment 80 Marcel de Rooy 2012-03-21 13:51:01 UTC
Created attachment 8410 [details] [review]
Bug 7310 Part 1: DB revision for Improving list permissions
Comment 81 Marcel de Rooy 2012-03-21 13:54:13 UTC
Created attachment 8411 [details] [review]
Bug 7310 Part 4: Code changes [main part]
Comment 82 Marcel de Rooy 2012-03-21 13:56:43 UTC
Created attachment 8412 [details] [review]
Bug 7310 Part 7: followup for sysprefs
Comment 83 Marcel de Rooy 2012-03-21 14:02:39 UTC
That's it for now. Hopefully the rebased patches will not be sleeping too long on the queue again. Have tested again and discovered another minor issue that already exists in master now that this report resolves on the staff side, but not on the opac side. Will not mention it here for simplicity. But will send another followup for it soon.
Back to Passed QA.
Comment 84 Paul Poulain 2012-03-21 16:05:35 UTC
seeing the size of those patches, it's hard to guarantee all coding guidelines are OK.
I tested it for a while, and could not find any problem.

Pushing before Feature Freeze, guys, we have one month to find a problem if there's one !

(switched to "enhancement" and versionned to 3.8, it's a bad idea to have that in 3.6 for now)
Comment 85 Marcel de Rooy 2012-03-22 08:35:31 UTC Comment hidden (obsolete)
Comment 86 Marcel de Rooy 2012-03-22 13:42:23 UTC
This screen shot is somewhat dubious: an owner is always allowed to add but not to delete. Just creating confusion..
Comment 87 Marcel de Rooy 2012-03-22 13:58:02 UTC
Created attachment 8519 [details]
Screenshot for new permissions

This reflects reality much better ;)
Comment 88 Paul Poulain 2012-03-22 16:14:02 UTC
I just pushed a small follow-up : it was written lt instead of < in DBrev number comparison, that made updatedatabase not working well.

Ppl running master should apply manually the following SQL:
ALTER TABLE virtualshelves MODIFY COLUMN owner int;
UPDATE virtualshelves vi LEFT JOIN borrowers bo ON bo.borrowernumber=vi.owner SET vi.owner=NULL where bo.borrowernumber IS NULL;
DELETE FROM virtualshelves WHERE owner IS NULL and category=1;
ALTER TABLE virtualshelves ADD COLUMN allow_add tinyint(1) DEFAULT 0, ADD COLUMN allow_delete_own tinyint(1) DEFAULT 1, ADD COLUMN allow_delete_other tinyint(1) DEFAULT 0, ADD CONSTRAINT `virtualshelves_ibfk_1` FOREIGN KEY (`owner`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL;
UPDATE virtualshelves SET allow_add=0, allow_delete_own=1, allow_delete_other=0 WHERE category=1;
UPDATE virtualshelves SET allow_add=0, allow_delete_own=1, allow_delete_other=0 WHERE category=2;
UPDATE virtualshelves SET allow_add=1, allow_delete_own=1, allow_delete_other=1 WHERE category=3;
UPDATE virtualshelves SET category=2 WHERE category=3;

ALTER TABLE virtualshelfcontents ADD COLUMN borrowernumber int, ADD CONSTRAINT `shelfcontents_ibfk_3` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL;
UPDATE virtualshelfcontents co LEFT JOIN virtualshelves sh USING (shelfnumber) SET co.borrowernumber=sh.owner;

CREATE TABLE virtualshelfshares
    (id int AUTO_INCREMENT PRIMARY KEY, shelfnumber int NOT NULL,
    borrowernumber int, invitekey varchar(10), sharedate datetime,
    CONSTRAINT `virtualshelfshares_ibfk_1` FOREIGN KEY (`shelfnumber`) REFERENCES `virtualshelves` (`shelfnumber`) ON DELETE CASCADE ON UPDATE CASCADE,
        CONSTRAINT `virtualshelfshares_ibfk_2` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES('OpacAllowPublicListCreation',1,'If set, allows opac users to create public lists',NULL,'YesNo');
INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES('OpacAllowSharingPrivateLists',0,'If set, allows opac users to share private lists with other patrons',NULL,'YesNo');
Comment 89 Chris Cormack 2012-11-05 22:36:17 UTC
We need the ability to add items to others lists now, because currently that is missing.

ie if you look at the screenshot, the options to share do not exist.
Comment 90 Chris Cormack 2012-11-05 22:50:25 UTC
Ignore that :) It is working as it should
Comment 91 Marcel de Rooy 2012-11-06 16:12:17 UTC
Actually, the work for this report is not finished yet.
My next step is a proposed patch for Share a list. The foundation is already there in code and tables.
Will open a new report for that step.