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?
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.
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.
(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.
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.
Created attachment 7092 [details] [review] Patch Fixing typo in opac-results.tt This makes the second and higher public list appear again in the AddTo combo of the search results screen. Please note that other work for this report will follow later ;)
Comment on attachment 7092 [details] [review] Patch Moved this patch to report 7459. It does not (really) belong here..
Created attachment 7678 [details] [review] [PATCH 1]DB revision for Improving list permissions When testing, use this patch to run updatedatabase. Note that open lists are converted to public lists with adequate permissions.
Created attachment 7679 [details] [review] [PATCH 2]Changing Koha SQL structure Can be tested by running kohastructure on a new database via command-line, or installing new Koha.
Created attachment 7680 [details] [review] [PATCH 3]New prefs on opac tab Check new prefs on OPAC tab, under Policy.
Created attachment 7681 [details] [review] [PATCH 4]Code revisions for improving permissions Mimics current functionality as much as possible (not yet releasing the new permissions on screen, and not allowing sharing yet). Will send a more detailed test plan via wiki. Testing mainly comes down to: Add new private and public lists, add and delete entries, delete list via OPAC and staff. Use shelves screen, search results and catalog detail screen. Do this under two different logins. Check what you can/cannot do with public lists from the other login.
Created attachment 7682 [details] [review] [PATCH 5]Deleting patron Test this by deleting a patron. Check if the public lists that he owned are still available, or if the entries he added to public lists are still there.
Submitted these patches yet, will add some more documentation, test plan via wiki soon.
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).
Created attachment 7769 [details] [review] [PATCH 6]Updated unit test Updated VirtualShelves unit test.
(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.
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.
(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?
(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.
Created attachment 7819 [details] [review] [PATCH 1][REVISED]DB revision for Improving list permissions
Created attachment 7820 [details] [review] [PATCH 2][REVISED]Changing Koha SQL structure
Created attachment 7821 [details] [review] [PATCH 4][REVISED]Code revisions for improving permissions
(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..
Created attachment 7823 [details] [review] [PATCH 4][REVISED]Code revisions for improving permissions
Please wait: Found something. Another version of patch 4 is coming..
Created attachment 7824 [details] [review] [PATCH 4][REVISED]Code revisions for improving permissions Changed defaults for new lists. Could also remove routine GetRecentShelves by using GetSomeShelfNames in catalogue/search.pl just as opac-search.pl already did. (More consistent.)
Created attachment 7825 [details] [review] [PATCH 5]Deleting patron Rebased.
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).
Wiki page http://wiki.koha-community.org/wiki/List_permissions also lists a few minor issues to be taken care of in a followup.
Created attachment 7828 [details] [review] [PATCH 7]Followup for warnings on somedeleted and nothingdeleted in staff client. This patch builds on top of patches 4 and 5. Test this patch by trying to delete items from a public list that someone else created (so you do not have sufficient permissions). Check the warning.
Created attachment 7873 [details] [review] Bug 7310: DB revision for Improving list permissions DB revision for improving list permissions. Adds new table virtualshelfshares for maintaining shared private lists. Adds three columns to virtualshelves for permissions per list. Adds column borrowernumber to virtualshelfcontents. Feb 16, 2012: Rebased. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7874 [details] [review] Bug 7310: Improving list permissions: Changing Koha SQL structure Modifying Koha structure for improving list permissions. Adds new table virtualshelfshares for maintaining shared private lists. Adds three columns to virtualshelves for permissions per list. Adds column borrowernumber to virtualshelfcontents. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7875 [details] [review] Bug 7310: Improving lists permissions -- New prefs Adds two new preferences to OPAC/Policy tab. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7876 [details] [review] Bug 7310: Code changes for Improving list permissions This lays the foundation for further changes for report 7310. Implements following points from the wiki page List permissions: 1) Preference that controls if users may create public lists in opac. 2) New add/delete own/delete other permissions per list. Code has been changed (in some cases refactored). New permissions are not yet visible; with this patch current functionality is kept as much as possible while resolving several issues, improving permissions and extending the code for further developments (using the new permissions and sharing lists). Feb 23, 2012 (revision): Changed defaults for new lists. Could also remove routine GetRecentShelves by using GetSomeShelfNames in catalogue/search.pl just as opac-search.pl already did. (More consistent.) Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7877 [details] [review] Bug 7310: Improving list permissions: Deleting patron Implements following points from the wiki page List permissions: 5) Delete or move list information when deleting a patron. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7878 [details] [review] Bug 7310: Updated VirtualShelves unit test Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7879 [details] [review] Bug 7310 Followup: Warnings for nothingdeleted and somedeleted for staff client. Test this patch by trying to delete items from a public list that someone else created (so you do not have sufficient permissions). Check the warning. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
jcamins: thx for testing and signing off!
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
(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.
(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.
(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.
Created attachment 7909 [details] [review] Bug 7310: Indentation followup Thanks Chris. Will look into it. This patch was created with mainly this line: while(s/^(\s*)\t/$1$spc/) {} Did the four files with the most changes.
Wrong version. Please wait..
Created attachment 7910 [details] [review] Bug 7310: Indentation followup Second try :-)
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 ..
Created attachment 7925 [details] [review] Bug 7310: DB revision for Improving list permissions Feb 28, 2012: Added three FK constraints for QA (for owner/borrowernumber in virtualshelves, shelfcontents and shelfshares). Tested update again. Also removed the note about public lists, made obsolete by earlier adjustments for comments from Owen.
Created attachment 7926 [details] [review] Bug 7310: Improving list permissions: Changing Koha SQL structure Feb 28, 2012: Added three FK constraints for QA (for owner/borrowernumber in virtualshelves, shelfcontents and shelfshares). Resolved syntax error with a comment. Tested the install.
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..
(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 ;)
Still checking another issue, please keep this report on HOLD now.
Created attachment 7939 [details] [review] Bug 7310: DB revision for Improving list permissions Rebased again.
Created attachment 7940 [details] [review] Bug 7310: Code changes for Improving list permissions Automerged version.
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.
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..
Created attachment 7942 [details] [review] Bug 7310: DB revision for Improving list permissions Small adjustment to make sure that adding the FK constraint is not frustrated by existing records in virtualshelves with invalid owner. Tested.
Created attachment 7953 [details] [review] Bug 7310: Followup for sysprefs.sql Resolving the omission to add the prefs to sysprefs.sql. *Sorry* Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested it separately by running the sql file on a new db. 324 entries now..
Created attachment 8006 [details] [review] Bug 7310: DB revision for Improving list permissions Rebased.
Comment on attachment 8006 [details] [review] Bug 7310: DB revision for Improving list permissions Wrong file
Created attachment 8007 [details] [review] Bug 7310: DB revision for Improving list permissions Rebased
Created attachment 8008 [details] [review] Bug 7310: Code changes for Improving list permissions Rebased for 4912 push
Created attachment 8009 [details] [review] Bug 7310 Indentation followup replacing leading tabs with spaces Rebased
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.
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?
(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.
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
Created attachment 8259 [details] [review] Bug 7310 Part 1: DB revision for Improving list permissions
Created attachment 8260 [details] [review] Bug 7310 Part 2: Improving Lists permissions new prefs
Created attachment 8261 [details] [review] Bug 7310 Part 3: improving list permissions changing Koha SQL structure
Created attachment 8262 [details] [review] Bug 7310 Part 4: Code changes [main part]
Created attachment 8263 [details] [review] Bug 7310 Part 5: deleting patron
Created attachment 8264 [details] [review] Bug 7310 Part 6: VirtualShelves unit test
Created attachment 8265 [details] [review] Bug 7310 Part 7: followup for sysprefs
Created attachment 8266 [details] [review] Bug 7310 Part 8: Indentation cleanup
Created attachment 8267 [details] [review] Bug 7310 Part 9: followup for warnings in nothingdeleted and somedeleted in staff client
Will send a followup under a new report number in order to not disturb further processsing of this set of patches. See bug 7732.
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 ?
Will rebase again.. What is the easiest way to get the patches again in the right order without git bz?
(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.
Will install git-bz on a new dev server soon.
Created attachment 8410 [details] [review] Bug 7310 Part 1: DB revision for Improving list permissions
Created attachment 8411 [details] [review] Bug 7310 Part 4: Code changes [main part]
Created attachment 8412 [details] [review] Bug 7310 Part 7: followup for sysprefs
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.
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)
Created attachment 8467 [details] Screen shot for new permissions
This screen shot is somewhat dubious: an owner is always allowed to add but not to delete. Just creating confusion..
Created attachment 8519 [details] Screenshot for new permissions This reflects reality much better ;)
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');
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.
Ignore that :) It is working as it should
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.