Bug 7310 introduced new list permissions in 3.8. Somehow these permissions were not clearly understood by everyone. Bug 10865 hid these permissions in various circumstances again in 3.16 (and 3.14). On a few reports we felt the need to simplify (bug 18214, 10679 as examples). This report proposes the following: [1] Allow three permissions (instead of the current 8 possibilities): N=Nobody changes list contents, O=Owner only makes changes, A=Anyone who sees the list, makes changes [2] The distinction between adding or deleting an entry is no longer made. If you now have change permission, you can do both. [3] Also deleting an entry does no longer depend on who added the entry. This especially confused users. [4] Formerly, the owner could always add entries. It is now possible to make a list readonly (again; concept before 3.8 iirc).
Created attachment 60939 [details] [review] Bug 18228: Table revision of virtualshelves In order to make the permissions easier, we will replace the columns allow_add, allow_delete_own and allow_delete_other by two new columns allow_change_from_owner and allow_change_from_others. The distinction between adding or deleting an entry is no longer made. If you have change permission, you can do both. Also deleting an entry does no longer depend on who added the entry. Formerly, the owner could always add entries. It is now possible to make a list readonly. We will not use the combination of owner=no and other=yes. This will leave us three possibilities: [1] owner=no, other=no: The list is read-only. No one can change contents of the list. Naturally, the owner can edit permissions. [2] owner=yes, other=no: Only the owner can change contents. [3] owner=yes, other=yes: Anyone seeing the list can change contents. This especially applies to shared lists and public lists. The two database columns will be presented in the interface as one permission field offering the three abovementioned options. Test plan: [1] Run the db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60940 [details] [review] Bug 18228: DBIx schema changes for Virtualshelve.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60941 [details] [review] Bug 18228: Implement the new columns in code The two new columns as mentioned in the commit message of the table revision must be used in the codebase now. Highlighting some changes in Koha::VirtualShel[f|ves]: [1] Additional methods is_public and is_private. [2] Method add_biblio did not check permissions. Does now. No impact on the interface, but one call in the unit test was affected. [3] Method remove_biblios is signficantly simplified. Removed a FIXME. [4] Method can_biblios_be_removed now redirects to can_biblios_be_added. A followup report may deal with unifying those routines. [5] Condition in get_some_shelves changed. [6] The reference to allow_add in get_shelves_containing_record can simply be removed. opac-shelves.pl and shelves.pl now pass the default setting of Owner only to the template. Templates shelves.tt and opac-shelves.tt now include the new permission field with three choices as mentioned in the table revision patch. opac-addbybiblionumber.pl and addbybiblionumber now need a check on allow_change_from_owner; search conditions slightly adjusted to the new permission scheme. Test plan: When we refer to visibility in the test plan, please check the Add to-combo on opac search results and staff results. And check opac-addbybiblionumber by clicking Save to Lists from opac results. The step 'Check delete' means: open the list in opac and check if you see the Delete button below the entries (only check, do not delete). [ 1] Create private list I01 (perm=Owner) [ 2] Check visibility: Seen. [ 3] Add a book. (Change by owner should be allowed.) [ 4] Check delete: Yes. [ 5] Edit list I01, set perm=Nobody [ 6] Check visibility: Not seen. [ 7] Check delete: No. [ 8] Share list I01 with another patron. [ 9] Check visibility for the other patron: Not seen. [10] Check delete for the other patron: No. [11] Change permission of list I01 to Anyone (by owner). [12] Check visibility for the other patron: Seen. [13] Let other patron add a book (change is allowed). [14] Let owner delete the same book again (change allowed). [15] Create public list U01 (perm=Owner) [16] Check visibility: Seen. [17] Add a book. (Change by owner should be allowed.) [18] Login as other user. Check visibility: Not seen. Check delete: No. [19] Change permission of U01 to Nobody (by owner) [20] As owner: Check visibility: Not seen. Check delete: No. [21] As other user: Check visibility: Not seen. Check delete: No. [22] Create public list U02 (perm=Anyone) [23] Add a book by owner. [24] Delete the same book by other user. Add another book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60942 [details] [review] Bug 18228: Adjust Virtualshelves.t Test plan: Run t/db_dependent/Virtualshelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Owen and Katrin, please take a look here. Since you were involved on bug 10865, your feedback is appreciated. Others: please test !
Created attachment 60943 [details] New edit list form
Created attachment 61471 [details] [review] Bug 18228: Adjust Virtualshelves.t Test plan: Run t/db_dependent/Virtualshelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com>
(In reply to Eric from comment #8) > Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Thanks, Eric. I only see your signoff on the last patch ?
Created attachment 61475 [details] [review] Bug 18228: Table revision of virtualshelves In order to make the permissions easier, we will replace the columns allow_add, allow_delete_own and allow_delete_other by two new columns allow_change_from_owner and allow_change_from_others. The distinction between adding or deleting an entry is no longer made. If you have change permission, you can do both. Also deleting an entry does no longer depend on who added the entry. Formerly, the owner could always add entries. It is now possible to make a list readonly. We will not use the combination of owner=no and other=yes. This will leave us three possibilities: [1] owner=no, other=no: The list is read-only. No one can change contents of the list. Naturally, the owner can edit permissions. [2] owner=yes, other=no: Only the owner can change contents. [3] owner=yes, other=yes: Anyone seeing the list can change contents. This especially applies to shared lists and public lists. The two database columns will be presented in the interface as one permission field offering the three abovementioned options. Test plan: [1] Run the db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 61476 [details] [review] Bug 18228: DBIx schema changes for Virtualshelve.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 61477 [details] [review] Bug 18228: Implement the new columns in code The two new columns as mentioned in the commit message of the table revision must be used in the codebase now. Highlighting some changes in Koha::VirtualShel[f|ves]: [1] Additional methods is_public and is_private. [2] Method add_biblio did not check permissions. Does now. No impact on the interface, but one call in the unit test was affected. [3] Method remove_biblios is signficantly simplified. Removed a FIXME. [4] Method can_biblios_be_removed now redirects to can_biblios_be_added. A followup report may deal with unifying those routines. [5] Condition in get_some_shelves changed. [6] The reference to allow_add in get_shelves_containing_record can simply be removed. opac-shelves.pl and shelves.pl now pass the default setting of Owner only to the template. Templates shelves.tt and opac-shelves.tt now include the new permission field with three choices as mentioned in the table revision patch. opac-addbybiblionumber.pl and addbybiblionumber now need a check on allow_change_from_owner; search conditions slightly adjusted to the new permission scheme. Test plan: When we refer to visibility in the test plan, please check the Add to-combo on opac search results and staff results. And check opac-addbybiblionumber by clicking Save to Lists from opac results. The step 'Check delete' means: open the list in opac and check if you see the Delete button below the entries (only check, do not delete). [ 1] Create private list I01 (perm=Owner) [ 2] Check visibility: Seen. [ 3] Add a book. (Change by owner should be allowed.) [ 4] Check delete: Yes. [ 5] Edit list I01, set perm=Nobody [ 6] Check visibility: Not seen. [ 7] Check delete: No. [ 8] Share list I01 with another patron. [ 9] Check visibility for the other patron: Not seen. [10] Check delete for the other patron: No. [11] Change permission of list I01 to Anyone (by owner). [12] Check visibility for the other patron: Seen. [13] Let other patron add a book (change is allowed). [14] Let owner delete the same book again (change allowed). [15] Create public list U01 (perm=Owner) [16] Check visibility: Seen. [17] Add a book. (Change by owner should be allowed.) [18] Login as other user. Check visibility: Not seen. Check delete: No. [19] Change permission of U01 to Nobody (by owner) [20] As owner: Check visibility: Not seen. Check delete: No. [21] As other user: Check visibility: Not seen. Check delete: No. [22] Create public list U02 (perm=Anyone) [23] Add a book by owner. [24] Delete the same book by other user. Add another book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 61478 [details] [review] Bug 18228: Adjust Virtualshelves.t Test plan: Run t/db_dependent/Virtualshelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Thanks Jesse
So you are suggesting to... loose features?
(In reply to Jonathan Druart from comment #15) > So you are suggesting to... loose features? The title says clearly what it does: Make them easier to use and understand. Bug 10865 is a vivid illustration for that. The permissions to add or delete are merged into a change permission.
If we merge these two permissions, we are limiting the possibilities.
(In reply to Jonathan Druart from comment #17) > If we merge these two permissions, we are limiting the possibilities. Will still answer more fully, but the foregoing comments already explain why this is done. Do you have a specific objection?
I'd like to get users' opinions before removing features. I have no idea if these permissions are used or not. I agree that it is not very clear, but we can still make the interface more easier to use/understand instead of remove features.
From my perspective this simplification will make things easier and doesn't take away any functionality we use. What is the purpose of the "Allow changes to contents from - Nobody" option? Why is it useful to create a list which you cannot add to, only delete altogether? The combination of "Category - Private" and "Allow changes to contents from: -- Anyone seeing this list" is confusing since the staff client lacks the "share" feature. Without knowing that you *could* share the list from the OPAC, allowing others to change a private list doesn't make sense. It would be nice to get sharing working in the staff client.
(In reply to Owen Leonard from comment #20) > From my perspective this simplification will make things easier and doesn't > take away any functionality we use. > > What is the purpose of the "Allow changes to contents from - Nobody" option? > Why is it useful to create a list which you cannot add to, only delete > altogether? > > The combination of "Category - Private" and "Allow changes to contents from: > -- Anyone seeing this list" is confusing since the staff client lacks the > "share" feature. Without knowing that you *could* share the list from the > OPAC, allowing others to change a private list doesn't make sense. > > It would be nice to get sharing working in the staff client. Thanks for responding. Good to read that the simplification is not interpreted as feature loss. The "Allow changes from Nobody" locks a list: you cannot add, but you cannot delete too. (A contents change is an add or a delete.) Some people may find it comfortable that the list is protected from accidental/incidental changes. The combination of Private and "Anyone seeing" is indeed a bit confusing (altough still true). Might add a follow-up or a new report for it. Sharing in the staff client certainly is a new report. I can at least open one to get some more feedback.
(In reply to Jonathan Druart from comment #19) > I'd like to get users' opinions before removing features. I have no idea if > these permissions are used or not. > I agree that it is not very clear, but we can still make the interface more > easier to use/understand instead of remove features. In response to comment17 and comment19 I wrote the Koha mailing list and asked for feedback on this report.
Basically with this patch we are going to remove the "Let people adds stuffs to this list but do not let them remove stuffs from this list (unless added by themself)" option. If nobody uses it, this patch makes code and interface much more easy to read/understand.
(In reply to Marcel de Rooy from comment #22) > In response to comment17 and comment19 I wrote the Koha mailing list and > asked for feedback on this report. Second call on April 18
(In reply to Jonathan Druart from comment #23) > Basically with this patch we are going to remove the "Let people adds stuffs > to this list but do not let them remove stuffs from this list (unless added > by themself)" option. > > If nobody uses it, this patch makes code and interface much more easy to > read/understand. Agreed.
From my point of view this does change the behaviour in the meaningful way, so I am OK with it ;)
Can we move this further?
All looks good to me unless that it still sounds weird to loose this feature.
(In reply to Jonathan Druart from comment #28) > All looks good to me unless that it still sounds weird to loose this feature. You seem to be the only one that cares about it. What can I still do?
Nothing, but I would wait for the next release to push. Feature freeze is passed anyway.
Created attachment 63474 [details] [review] Bug 18228: Add missing comma in kohastructure.sql
*** Bug 18214 has been marked as a duplicate of this bug. ***
Rebasing this on top of 18214
Created attachment 64505 [details] [review] Bug 18228: Table revision of virtualshelves In order to make the permissions easier, we will replace the columns allow_add, allow_delete_own and allow_delete_other by two new columns allow_change_from_owner and allow_change_from_others. The distinction between adding or deleting an entry is no longer made. If you have change permission, you can do both. Also deleting an entry does no longer depend on who added the entry. Formerly, the owner could always add entries. It is now possible to make a list readonly. We will not use the combination of owner=no and other=yes. This will leave us three possibilities: [1] owner=no, other=no: The list is read-only. No one can change contents of the list. Naturally, the owner can edit permissions. [2] owner=yes, other=no: Only the owner can change contents. [3] owner=yes, other=yes: Anyone seeing the list can change contents. This especially applies to shared lists and public lists. The two database columns will be presented in the interface as one permission field offering the three abovementioned options. Test plan: [1] Run the db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 64506 [details] [review] Bug 18228: DBIx schema changes for Virtualshelve.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 64507 [details] [review] Bug 18228: Implement the new columns in code The two new columns as mentioned in the commit message of the table revision must be used in the codebase now. Highlighting some changes in Koha::VirtualShel[f|ves]: [1] Additional methods is_public and is_private. [2] Method add_biblio did not check permissions. Does now. No impact on the interface, but one call in the unit test was affected. [3] Method remove_biblios is signficantly simplified. Removed a FIXME. [4] Method can_biblios_be_removed now redirects to can_biblios_be_added. A followup report may deal with unifying those routines. [5] Condition in get_some_shelves changed. [6] The reference to allow_add in get_shelves_containing_record can simply be removed. opac-shelves.pl and shelves.pl now pass the default setting of Owner only to the template. Templates shelves.tt and opac-shelves.tt now include the new permission field with three choices as mentioned in the table revision patch. opac-addbybiblionumber.pl and addbybiblionumber now need a check on allow_change_from_owner; search conditions slightly adjusted to the new permission scheme. Test plan: When we refer to visibility in the test plan, please check the Add to-combo on opac search results and staff results. And check opac-addbybiblionumber by clicking Save to Lists from opac results. The step 'Check delete' means: open the list in opac and check if you see the Delete button below the entries (only check, do not delete). [ 1] Create private list I01 (perm=Owner) [ 2] Check visibility: Seen. [ 3] Add a book. (Change by owner should be allowed.) [ 4] Check delete: Yes. [ 5] Edit list I01, set perm=Nobody [ 6] Check visibility: Not seen. [ 7] Check delete: No. [ 8] Share list I01 with another patron. [ 9] Check visibility for the other patron: Not seen. [10] Check delete for the other patron: No. [11] Change permission of list I01 to Anyone (by owner). [12] Check visibility for the other patron: Seen. [13] Let other patron add a book (change is allowed). [14] Let owner delete the same book again (change allowed). [15] Create public list U01 (perm=Owner) [16] Check visibility: Seen. [17] Add a book. (Change by owner should be allowed.) [18] Login as other user. Check visibility: Not seen. Check delete: No. [19] Change permission of U01 to Nobody (by owner) [20] As owner: Check visibility: Not seen. Check delete: No. [21] As other user: Check visibility: Not seen. Check delete: No. [22] Create public list U02 (perm=Anyone) [23] Add a book by owner. [24] Delete the same book by other user. Add another book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 64508 [details] [review] Bug 18228: Adjust Virtualshelves.t Test plan: Run t/db_dependent/Virtualshelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 64509 [details] [review] Bug 18228: Add missing comma in kohastructure.sql Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Owen Leonard from comment #20) > The combination of "Category - Private" and "Allow changes to contents from: > -- Anyone seeing this list" is confusing since the staff client lacks the > "share" feature. Without knowing that you *could* share the list from the > OPAC, allowing others to change a private list doesn't make sense. On bug 18834 I am removing the Anyone permission for strictly private lists and in staff I am adding a Type column that shows Shared for private lists with shares just like OPAC does already.
Just a summary of the current status: Although Jonathan planned to push this patch set shortly after release of 17.05, he still feels that it needs another QA opinion. As can be seen in the previous comments, I asked feedback (repeatedly) on the community and the dev mailing list. Apart from the signoff by Jesse M and Eric G, positive comments have been added by Owen L, Marc V and Josef M. The only concern is from Jonathan about "losing a feature". In short, this report simplifies the scheme of three options Allow add, Allow delete own entries and Allow delete other entries (with 2^3=8 possible combinations) by one option "Allow changes from" with three choices: Nobody, Owner or "Anyone seeing the list". What we actually remove, is the distinction between Add and Delete. We replace it by Change. If you are allowed to change, you can add and/or delete entries. The concept is easier and nobody expressed interest in keeping it. Allowing only adds or only deletes was mostly theoretically interesting and probably not used. The distinction between Delete own and Delete other was confusing, since Koha does not show who added an entry. It is not that interesting too probably. If you are allowed to change, you can delete any entry now. Additionally, we now have the Nobody option. This allows you to make a list read-only. Even the owner cannot change. This may prevent accidental deletes. Bug 18834 builds on top of this patch set and hides the Anyone permission for strictly private lists. Dynamically alters the permission combo. Fixes a bug for empty shelf name. And adds a Shared indication for a list in staff. Please help to get this further.
Created attachment 64780 [details] [review] Bug 18228: Table revision of virtualshelves In order to make the permissions easier, we will replace the columns allow_add, allow_delete_own and allow_delete_other by two new columns allow_change_from_owner and allow_change_from_others. The distinction between adding or deleting an entry is no longer made. If you have change permission, you can do both. Also deleting an entry does no longer depend on who added the entry. Formerly, the owner could always add entries. It is now possible to make a list readonly. We will not use the combination of owner=no and other=yes. This will leave us three possibilities: [1] owner=no, other=no: The list is read-only. No one can change contents of the list. Naturally, the owner can edit permissions. [2] owner=yes, other=no: Only the owner can change contents. [3] owner=yes, other=yes: Anyone seeing the list can change contents. This especially applies to shared lists and public lists. The two database columns will be presented in the interface as one permission field offering the three abovementioned options. Test plan: [1] Run the db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64781 [details] [review] Bug 18228: DBIx schema changes for Virtualshelve.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64782 [details] [review] Bug 18228: Implement the new columns in code The two new columns as mentioned in the commit message of the table revision must be used in the codebase now. Highlighting some changes in Koha::VirtualShel[f|ves]: [1] Additional methods is_public and is_private. [2] Method add_biblio did not check permissions. Does now. No impact on the interface, but one call in the unit test was affected. [3] Method remove_biblios is signficantly simplified. Removed a FIXME. [4] Method can_biblios_be_removed now redirects to can_biblios_be_added. A followup report may deal with unifying those routines. [5] Condition in get_some_shelves changed. [6] The reference to allow_add in get_shelves_containing_record can simply be removed. opac-shelves.pl and shelves.pl now pass the default setting of Owner only to the template. Templates shelves.tt and opac-shelves.tt now include the new permission field with three choices as mentioned in the table revision patch. opac-addbybiblionumber.pl and addbybiblionumber now need a check on allow_change_from_owner; search conditions slightly adjusted to the new permission scheme. Test plan: When we refer to visibility in the test plan, please check the Add to-combo on opac search results and staff results. And check opac-addbybiblionumber by clicking Save to Lists from opac results. The step 'Check delete' means: open the list in opac and check if you see the Delete button below the entries (only check, do not delete). [ 1] Create private list I01 (perm=Owner) [ 2] Check visibility: Seen. [ 3] Add a book. (Change by owner should be allowed.) [ 4] Check delete: Yes. [ 5] Edit list I01, set perm=Nobody [ 6] Check visibility: Not seen. [ 7] Check delete: No. [ 8] Share list I01 with another patron. [ 9] Check visibility for the other patron: Not seen. [10] Check delete for the other patron: No. [11] Change permission of list I01 to Anyone (by owner). [12] Check visibility for the other patron: Seen. [13] Let other patron add a book (change is allowed). [14] Let owner delete the same book again (change allowed). [15] Create public list U01 (perm=Owner) [16] Check visibility: Seen. [17] Add a book. (Change by owner should be allowed.) [18] Login as other user. Check visibility: Not seen. Check delete: No. [19] Change permission of U01 to Nobody (by owner) [20] As owner: Check visibility: Not seen. Check delete: No. [21] As other user: Check visibility: Not seen. Check delete: No. [22] Create public list U02 (perm=Anyone) [23] Add a book by owner. [24] Delete the same book by other user. Add another book. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64783 [details] [review] Bug 18228: Adjust Virtualshelves.t Test plan: Run t/db_dependent/Virtualshelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64784 [details] [review] Bug 18228: Add missing comma in kohastructure.sql Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64785 [details] [review] QA Followup - use gender neutral language in new tests
(In reply to Nick Clemens from comment #45) > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Great! Thanks, Nick.
Pushed to master for 17.11, thanks to everybody involved!
Enhancement not pushed to 17.05.x