Bug 26346 - Add option to make a public list editable by library staff only
Summary: Add option to make a public list editable by library staff only
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Nick Clemens
URL:
Keywords:
: 28791 (view as bug list)
Depends on:
Blocks: 30418
  Show dependency treegraph
 
Reported: 2020-09-02 06:07 UTC by Henry Bolshaw
Modified: 2023-12-28 20:42 UTC (History)
19 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/658
Text to go in the release notes:
Add a new option for staff users to manage the contents of public lists from the staff client and OPAC. This patchset also allows superlibrarian users, or those with the 'edit_public_lists' sub-permission, to edit the configuration of existing public lists.
Version(s) released in:
22.05.00


Attachments
Bug 26346: Add option to make public lists editable by all staff (12.18 KB, patch)
2021-04-15 21:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Updated Edit list hint message (4.89 KB, patch)
2021-05-07 09:22 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to Remove items from list (1.41 KB, patch)
2021-05-07 09:23 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Move checking if patron can change staff list to Koha/Patron.pm (5.01 KB, patch)
2021-05-07 09:23 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Fix unit tests (17.58 KB, patch)
2021-05-13 01:36 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (34.25 KB, patch)
2021-05-13 05:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (36.74 KB, patch)
2021-05-14 00:04 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (36.80 KB, patch)
2021-05-14 01:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26346: Add edit_public_list permission enabling patrons with the permission enabled to switch 'owner only' public lists to 'staff only' (23.53 KB, patch)
2021-05-31 01:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.55 KB, patch)
2021-05-31 01:21 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (36.83 KB, patch)
2021-06-16 11:45 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.59 KB, patch)
2021-06-16 11:45 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.04 KB, patch)
2021-07-02 09:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26346: Database changes (1.03 KB, patch)
2021-07-02 11:14 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.47 KB, patch)
2021-07-02 11:15 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (34.25 KB, patch)
2021-07-02 11:15 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.61 KB, patch)
2021-07-02 11:16 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.06 KB, patch)
2021-07-02 11:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (8.04 KB, patch)
2021-07-02 11:18 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (34.16 KB, patch)
2021-07-09 04:51 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.61 KB, patch)
2021-07-09 04:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.06 KB, patch)
2021-07-09 04:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (8.97 KB, patch)
2021-07-09 04:55 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (9.80 KB, patch)
2021-07-09 05:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (9.42 KB, patch)
2021-07-09 06:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (2.04 KB, patch)
2021-08-23 06:40 UTC, Alex Buckley
Details | Diff | Splinter Review
Public list with "Allow changes to contents from=Anyone seeing this list" shows to anonymous patron in Add to a list page (41.73 KB, image/png)
2021-08-23 06:52 UTC, Alex Buckley
Details
Bug 26346: (follow-up) Fixes in response to QA (2.16 KB, patch)
2021-09-28 21:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (21.67 KB, patch)
2021-09-30 06:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (34.23 KB, patch)
2021-10-07 21:12 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.61 KB, patch)
2021-10-07 21:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.06 KB, patch)
2021-10-07 21:15 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (21.67 KB, patch)
2021-10-07 21:15 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.71 KB, patch)
2021-11-12 02:46 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (33.78 KB, patch)
2021-11-12 02:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.61 KB, patch)
2021-11-12 02:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.06 KB, patch)
2021-11-12 02:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (21.67 KB, patch)
2021-11-12 02:55 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Database changes (1.03 KB, patch)
2021-11-25 20:45 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.72 KB, patch)
2021-11-25 20:46 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (33.78 KB, patch)
2021-11-25 20:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists (23.61 KB, patch)
2021-11-25 20:52 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user (1.06 KB, patch)
2021-11-25 20:52 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (21.67 KB, patch)
2021-11-25 20:52 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixes in response to QA (21.67 KB, patch)
2021-11-25 21:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Database changes (2.84 KB, patch)
2021-12-10 04:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.72 KB, patch)
2021-12-10 04:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.68 KB, patch)
2021-12-10 04:09 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.61 KB, patch)
2022-03-15 00:41 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.74 KB, patch)
2022-03-15 00:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.58 KB, patch)
2022-03-15 18:07 UTC, Blou
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.72 KB, patch)
2022-03-16 03:49 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixing for QA tool (2.35 KB, patch)
2022-03-28 22:12 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 26346: Database changes (2.90 KB, patch)
2022-03-29 16:22 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.78 KB, patch)
2022-03-29 16:22 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.78 KB, patch)
2022-03-29 16:22 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixing for QA tool (2.40 KB, patch)
2022-03-29 16:22 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixing for QA tool (2.40 KB, patch)
2022-03-29 16:24 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26346: Database changes (2.96 KB, patch)
2022-03-31 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26346: Updated schema file (1.83 KB, patch)
2022-03-31 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26346: Add option to make public lists editable by all staff (44.83 KB, patch)
2022-03-31 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26346: (follow-up) Fixing for QA tool (2.46 KB, patch)
2022-03-31 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26346: (QA follow-up) Clarify permission (1.11 KB, patch)
2022-03-31 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Henry Bolshaw 2020-09-02 06:07:56 UTC
Currently there are three options for who can add or remove items from a public list:
- "Nobody"
- "Owner Only"
- "Anybody seeing this list"

It would be useful to have a "staff only" option as well so that all users could see the list but only library staff could add or remove items from the list.

At the moment we have to either rely on a single member of staff to update and maintain a public list or give everyone edit rights and risk users accidentally or deliberately editing a list.
Comment 1 Rhonda Kuiper 2020-09-24 14:31:04 UTC
Having an additional option so that all users could see the list but only library staff could add or remove items would be WONDERFUL!  Our Youth Services department would be most grateful.
Comment 2 Jennifer Marlatt 2020-09-25 18:45:35 UTC
This would be fantastic! When a staff member leaves we have to recreate their lists so we can update the list as needed.
Comment 3 Laura Horgan 2020-12-17 15:26:20 UTC
This I believe is exactly what I am looking for.... A member of our public services team created a list and currently, I ( in tech services) am unable when new titles are added to edit this list.  I would like this to be linked to not only staff but the staff in my organization as we are in a multi-library group.  Several libraries have created a local list of their new selections of DVDs, etc. and we would want to make sure that permission to change the list was limited to the institution who created the lists -- further modified by those on our staff that have the permission to add, edit, or delete lists.
Comment 4 Alex Buckley 2021-04-15 21:35:18 UTC Comment hidden (obsolete)
Comment 5 Henry Bolshaw 2021-04-27 22:49:50 UTC
Thanks Alex, this is a real improvement! I followed the test plan and everything worked as you said it would.

Option to Remove items from list?
-----------------------
This patch allows other library staff to add items to the list but it doesn't give them the ability to remove items. Is this functionality you are planning to add as a subsequent patch or do you see that as a separate bug? Without the ability to remove items, the interface is slightly confusing because both the OPAC and staff interface still give library staff the option to remove items - but if they try then there's an error, "no record was removed".


Update Edit list hint message
-----------------------
On the staff interface when creating or editing a list, if you select "Allow changes to contents from: Staff Only" while the category is still private, you get this hint: "The Anyone permission has no actual effect while this list is strictly private." Can you tweak this to reflect the new staff permission options?

No staff only option in OPAC
---------------------
The staff only option doesn't appear in the OPAC, so if the list owner edits a list from the OPAC - e.g. to edit the name - the list permissions will change. I don't know if this is easily fixable because you wouldn't want the staff only option appearing for all users but I thought I'd mention it.
Comment 6 Rhiana 2021-04-28 16:46:03 UTC
We would use this 100% of the time. Most of our 'public' lists are only public to share with staff and work on collaboratively.
Comment 7 Alex Buckley 2021-05-07 09:22:59 UTC Comment hidden (obsolete)
Comment 8 Alex Buckley 2021-05-07 09:23:28 UTC Comment hidden (obsolete)
Comment 9 Alex Buckley 2021-05-07 09:23:55 UTC Comment hidden (obsolete)
Comment 10 Alex Buckley 2021-05-07 09:26:02 UTC
(In reply to Henry Bolshaw from comment #5)
> Thanks Alex, this is a real improvement! I followed the test plan and
> everything worked as you said it would.
> 
Thanks Henry! Apologies for the delay getting back to you I've been moving house.

I've attached patches addressing two of your points below, and a third patch refactoring the code - creating a new subroutine in Koha/Patron.pm to remove code duplication.
>
> Option to Remove items from list?
> -----------------------
> This patch allows other library staff to add items to the list but it
> doesn't give them the ability to remove items. Is this functionality you are
> planning to add as a subsequent patch or do you see that as a separate bug?
> Without the ability to remove items, the interface is slightly confusing
> because both the OPAC and staff interface still give library staff the
> option to remove items - but if they try then there's an error, "no record
> was removed".
> 
> 
This should have been added by one of the patches I have attached.
>
> Update Edit list hint message
> -----------------------
> On the staff interface when creating or editing a list, if you select "Allow
> changes to contents from: Staff Only" while the category is still private,
> you get this hint: "The Anyone permission has no actual effect while this
> list is strictly private." Can you tweak this to reflect the new staff
> permission options?
> 
This should also have been added in one of my new patches.
>
> No staff only option in OPAC
> ---------------------
> The staff only option doesn't appear in the OPAC, so if the list owner edits
> a list from the OPAC - e.g. to edit the name - the list permissions will
> change. I don't know if this is easily fixable because you wouldn't want the
> staff only option appearing for all users but I thought I'd mention it.
> 
I completely agree with you. I'm not 100% sure how to go about fixing this. Perhaps it could be a follow-up fix?

I am part way through another patch for this bug report adding unit test coverage, that will hopefully be finished sometime next week. So I'll keep the status as 'Assigned' for now.
Comment 11 Alex Buckley 2021-05-13 01:36:30 UTC Comment hidden (obsolete)
Comment 12 Alex Buckley 2021-05-13 05:08:25 UTC Comment hidden (obsolete)
Comment 13 Alex Buckley 2021-05-13 05:11:31 UTC
Hi Henry, 

I've rebased all my commits into a single patch to reduce down to one test plan. I've also added unit test coverage in t/db_dependent/Virtualshelves.t and t/db_dependent/Koha/Patron.t

The fixes I mentioned in comment #10 are in my rebased patch. 

Could you please re-test now?

Many thanks,
Alex
Comment 14 Andrew Fuerste-Henry 2021-05-13 11:57:37 UTC
This works great up until step 23 of your test plan. I'm not getting the "permission has no actual effect while this list is strictly private" message on the OPAC when I pick Staff or Anyone for a private list.
Comment 15 Alex Buckley 2021-05-14 00:04:24 UTC Comment hidden (obsolete)
Comment 16 Alex Buckley 2021-05-14 00:07:45 UTC
Hi Andrew and Henry, 

Thanks for testing Andrew. I found you weren't getting the "permission has no actual effect while this list is strictly private" message in the OPAC due to a JavaScript error. I think that should be fixed up in the patch I've just attached, 

Henry, I think I've also addressed your "No staff only option in OPAC" point (from  comment #5), and have added a step 25 to the test plan on my new patch to cover testing that.

Can you please re-test now?

Many thanks,
Alex
Comment 17 Andrew Fuerste-Henry 2021-05-14 01:05:42 UTC Comment hidden (obsolete)
Comment 18 Andrew Fuerste-Henry 2021-05-14 01:07:05 UTC
Thanks for the quick fix! I hated to fail it on something so small right at the end. This will be a really great change, I have librarians ask about it all the time!
Comment 19 Alex Buckley 2021-05-14 01:08:01 UTC
(In reply to Andrew Fuerste-Henry from comment #18)
> Thanks for the quick fix! I hated to fail it on something so small right at
> the end. This will be a really great change, I have librarians ask about it
> all the time!

No problem at all Andrew. Thanks for testing!
Comment 20 Henry Bolshaw 2021-05-17 21:07:34 UTC
> Henry, I think I've also addressed your "No staff only option in OPAC" point
> (from  comment #5), and have added a step 25 to the test plan on my new
> patch to cover testing that.
> 
> Can you please re-test now?
> 
> Many thanks,
> Alex

Yes, this all works well on the OPAC now. Thanks!
Comment 21 Alex Buckley 2021-05-31 01:17:26 UTC Comment hidden (obsolete)
Comment 22 Alex Buckley 2021-05-31 01:18:32 UTC
Hi Andrew and Henry, 

Thank you both for your testing of this enhancement so far!

We spotted a potential problem with the first patch: If your library has a public 'owner only' list and you want to switch it to being a 'staff only' list then you have to ask the list owner to make this switch.

If the list owner has left, or is unavailable, then it's not possible to make this switch. 

So I've just written a follow-up second patch. It adds a new patron sub-permission: 'edit_public_lists'. When enabled a patron can edit a public 'owner only' list, switching it to a 'staff only' list.

Could you please test my second patch (which has it's own test plan)?

Many thanks,
Alex
Comment 23 Alex Buckley 2021-05-31 01:21:20 UTC Comment hidden (obsolete)
Comment 24 Alex Buckley 2021-05-31 01:22:38 UTC
Sorry, just had to make a quick correction to the commit text of the second patch. The second patch is all ready for testing now :)
Comment 25 Alex Buckley 2021-06-16 10:21:57 UTC
Hi Henry and Andrew, 

Just wondering if either of you have had time to re-test the patchset with the follow-up patch in place now?

Thanks,
Alex
Comment 26 Henry Bolshaw 2021-06-16 10:29:06 UTC
Sorry Alex, I've not had a chance to look yet but I'll try and do some testing this week. The new functionality sound great though!
Comment 27 Andrew Fuerste-Henry 2021-06-16 11:45:53 UTC Comment hidden (obsolete)
Comment 28 Andrew Fuerste-Henry 2021-06-16 11:45:57 UTC Comment hidden (obsolete)
Comment 29 Alex Buckley 2021-06-17 20:17:10 UTC
Thanks Henry and Andrew for your continued interest in this patchset, I really appreciate it!

Thanks for the test and signoff Andrew!
Comment 30 Marcel de Rooy 2021-07-02 08:33:47 UTC
QAing
Comment 31 Marcel de Rooy 2021-07-02 08:34:19 UTC
 FAIL   opac/opac-shelves.pl
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
Comment 32 Marcel de Rooy 2021-07-02 08:34:36 UTC
 FAIL   t/db_dependent/Virtualshelves.t
   OK     critic
   FAIL   forbidden patterns
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 384)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 389)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 394)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 399)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 404)
Comment 33 Marcel de Rooy 2021-07-02 08:56:56 UTC
Anonymous OPAC user

Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253

Surely, I do not like that :)
Comment 34 Marcel de Rooy 2021-07-02 09:08:22 UTC
diff --git a/Koha/Schema/Result/Letter.pm b/Koha/Schema/Result/Letter.pm
index 613c4d9b27..b5627b9674 100644
--- a/Koha/Schema/Result/Letter.pm
+++ b/Koha/Schema/Result/Letter.pm
@@ -29,8 +29,6 @@ __PACKAGE__->table("letter");
   is_auto_increment: 1
   is_nullable: 0

-primary key identifier
-
 =head2 module

   data_type: 'varchar'
@@ -217,8 +215,8 @@ __PACKAGE__->belongs_to(
 );


-# Created by DBIx::Class::Schema::Loader v0.07046 @ 2021-02-11 12:33:50
-# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:qqdTVEicMu5rHppY5qsEuA
+# Created by DBIx::Class::Schema::Loader v0.07046 @ 2021-04-09 22:58:34
+# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:2QkUwMWgZ9WfOk9Ngf7yFA

Dont ever do these things in a regular patch !
Please move such changes to separate patches.
In this case I wonder what you are doing..
Comment 35 Marcel de Rooy 2021-07-02 09:14:03 UTC
Line 190
 Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists )
Line 213
Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists

Wonder if we could have a crash here too?
Comment 36 Marcel de Rooy 2021-07-02 09:16:13 UTC
@@ -267,4 +270,3 @@ sub _type {
     return 'Virtualshelve';
 }

-1;

Never never do this!
Comment 37 Marcel de Rooy 2021-07-02 09:18:03 UTC
Virtualshelves

+        if ( Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) {

Could this crash?
Comment 38 Marcel de Rooy 2021-07-02 09:19:11 UTC
+++ b/Koha/Virtualshelves.pm

+use C4::Auth;
+

Out of the blue? Why are you adding it ??
Comment 39 Marcel de Rooy 2021-07-02 09:25:03 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2021-07-02 09:27:03 UTC
Sorry but there is really too much to fix on this small patch set to get it further now.
Including serious errors like removing a true value, adding DBIx changes in a patch, using modules unrelated.

Please carefully adjust !
Comment 41 Marcel de Rooy 2021-07-02 09:32:47 UTC
And still forgot:

The Staff only permission has no actual effect while this list is strictly private.
=> Should we not allow that change then ?


CHECK Handling 0 and NULL in column allow_change_from_staff
I saw in my table a NULL and a zero there. Probably existing/new rec.


This is a bit teasing: But I cannot add an item to a public list from another staff member. But I can edit the list, change the 'Allow', add or delete items, and undo Allow. Just noting.
Comment 42 Alex Buckley 2021-07-02 11:14:54 UTC
Created attachment 122535 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table

Sponsored-By: Catalyst IT
Comment 43 Alex Buckley 2021-07-02 11:15:18 UTC
Created attachment 122536 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT
Comment 44 Alex Buckley 2021-07-02 11:15:40 UTC Comment hidden (obsolete)
Comment 45 Alex Buckley 2021-07-02 11:16:57 UTC Comment hidden (obsolete)
Comment 46 Alex Buckley 2021-07-02 11:17:26 UTC Comment hidden (obsolete)
Comment 47 Alex Buckley 2021-07-02 11:18:03 UTC Comment hidden (obsolete)
Comment 48 Alex Buckley 2021-07-02 11:56:43 UTC
(In reply to Marcel de Rooy from comment #40 and comment #41)

Hi Marcel,

Thank you for QAing!

- Comments #31, #32, #35, #36, #37, #38 (which was an accident- apologies!) should hopefully be addressed by the 'Bug 26346: (follow-up) Fixes in response to QA' patch.

- Comment #34 should hopefully have been addressed by my splitting the database change + DBIx change from the 'Bug 26346: Add option to make public lists editable by all staff' patch out into two separate patches.
Note: The change to Letter.pm you noted in comment #34 was also incorrectly committed and is not in the new 'Bug 26346: Updated schema file' patch.

Regarding comment #41:

1. I can certainly add that, however, my thoughts on this are: If a list is private I would have thought it was expected that other staff members couldn't see/edit/delete it. Thereby making it justified that the Staff only permission has not effect while the list is private.

What do you think?

2. Existing lists will have allow_change_from_staff = 0 due to that being the default value of the new column - https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122535&action=diff

The use of both 0 and NULL in allow_change_from_staff also happens in allow_change_from_others. This is what I observed when creating and adding items to lists:

* When creating a public 'anyone seeing this list' list allow_change_from_staff = NULL. When I add an item to the list then allow_change_from_staff = 0

* When creating a public 'staff only' list allow_change_from_others = NULL. When I add an item to the list then allow_change_from_others = 0

Similar behaviour exists if you test on master too:

* Create a public 'owner only' list allow_change_from_others = NULL. When I add an item to the list then allow_change_from_others = 0

3. Would you mind elaborating on if this is in the staff client and/or OPAC? I haven't been able to replicate so not sure if I'm doing something wrong.

Putting this in 'Needs signoff', apologies if that's the incorrect status for the bug report at this point!

Alex
Comment 49 Marcel de Rooy 2021-07-08 14:12:28 UTC
Good to see these changes.
We still need some attention here:

sub get_some_shelves {
    my ( $self, $params ) = @_;
    my $borrowernumber = $params->{borrowernumber} || 0;
    my $category = $params->{category} || 1;
    my $add_allowed = $params->{add_allowed};

    my @conditions;
    my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
    if ( $add_allowed ) {

At this point we prevented a crash but now the side-effect is that we wont see any public lists at all when not logged in..
Comment 50 Alex Buckley 2021-07-08 21:14:51 UTC
(In reply to Marcel de Rooy from comment #49)
> Good to see these changes.
> We still need some attention here:
> 
> sub get_some_shelves {
>     my ( $self, $params ) = @_;
>     my $borrowernumber = $params->{borrowernumber} || 0;
>     my $category = $params->{category} || 1;
>     my $add_allowed = $params->{add_allowed};
> 
>     my @conditions;
>     my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
>     if ( $add_allowed ) {
> 
> At this point we prevented a crash but now the side-effect is that we wont
> see any public lists at all when not logged in..

Thanks Marcel!

Working on this now.
Comment 51 Alex Buckley 2021-07-09 04:51:45 UTC
Created attachment 122685 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 52 Alex Buckley 2021-07-09 04:54:02 UTC
Created attachment 122686 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 53 Alex Buckley 2021-07-09 04:54:46 UTC
Created attachment 122687 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Alex Buckley 2021-07-09 04:55:20 UTC
Created attachment 122688 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Sponsored-By: Catalyst IT
Comment 55 Alex Buckley 2021-07-09 05:25:20 UTC
Created attachment 122689 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Test plan for comment #49 fix:

1. Create a 'staff only' public list and 'Anyone seeing this list'
public list

2. Visit the OPAC. Do not login. Confirm you are able to see both lists
created in comment #1 in the OPAC

3. Perform a search, select a search result checkbox and
view the dropdown options in the 'Select titles to:' dropdown

3. Confirm both lists in step #1 are displayed in the dropdown

4. Try adding an item to the 'staff only' list. Observe you're prompted
to login

5. Login as a user with staff client permissions and confirm you can add
the item to the 'staff only' list

6. Logout, and repeat step 4. Login as a user with no permissionsi.
Confirm you cannot add an item to a 'staff only' list

7. Confirm both staff users and users with no permissions can add items to the 'Anyone seeing the list' public list in the OPAC

Sponsored-By: Catalyst IT
Comment 56 Alex Buckley 2021-07-09 06:24:29 UTC
I haven't quite finished this yet. 
Have re-thought the get_some_shelves() change. Amended patch to come shortly.
Comment 57 Alex Buckley 2021-07-09 06:47:46 UTC
Created attachment 122691 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client). Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the dropdown

Sponsored-By: Catalyst IT
Comment 58 Alex Buckley 2021-07-09 06:57:21 UTC
(In reply to Marcel de Rooy from comment #49)

Hi Marcel, 

1. I've rebased the third patch 'Bug 26346: Add option to make public lists editable by all staff'. It conflicted when applied on master.

2. Have amended the sixth patch 'Bug 26346: (follow-up) Fixes in response to QA' to address comment #49 - along with a test plan for the comment #49 fix in case that's something you wanted.

- All types of public lists are now displayed in the 'Lists' dropdown in the OPAC header to both un-authenticated and authenticated users

- When an un-authenticated user (i.e. not logged in) selects an item and tries to see what lists they can add said item to then only "Anyone seeing this list" public lists show as available.

- 'Owner only' and staff only' lists are only displayed as lists which can be added to if the OPAC user is A) Logged in and B) Permitted to add to those lists.

This is essentially the same behaviour as happens on master with regards to 'owner only' public lists.

Ready for review.

Alex
Comment 59 Marcel de Rooy 2021-08-09 14:18:05 UTC
sub can_be_deleted {
    my ( $self, $borrowernumber ) = @_;

    return 0 unless $borrowernumber;
    return 1 if $self->owner == $borrowernumber;

    my $patron = Koha::Patrons->find( $borrowernumber );

    return 1 if $self->is_public and haspermission( $patron->userid, { lists => 'delete_public_lists' } );

    return 0;
}

This is still different from surrounding code as to testing Patrons::find.
Comment 60 Marcel de Rooy 2021-08-09 14:27:38 UTC
can_biblios_be_added

if $borrowernumber
      and ( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) or ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) or $self->allow_change_from_others );

Isnt it actually strange that you could be the owner of a list, being a staff member, but without the staff_only_lists perm, and you cannot even add or remove one book to your own list ?

Note that as owner you still can manage or delete the whole list!
Do we need a more clear definition of what "staff only" actually means?
I would think more of an extension to staff users..
Comment 61 Marcel de Rooy 2021-08-09 14:28:10 UTC
add_biblio

 my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
    return unless ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) || ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) || $self->allow_change_from_others;

Why return 0 on the first line and return undef on the second?
Comment 62 Marcel de Rooy 2021-08-09 14:30:50 UTC
(In reply to Alex Buckley from comment #58)
> - When an un-authenticated user (i.e. not logged in) selects an item and
> tries to see what lists they can add said item to then only "Anyone seeing
> this list" public lists show as available.

Anyone seeing this list is not really true.
Anonymous cant add or delete entries.
Comment 63 Marcel de Rooy 2021-08-09 14:31:29 UTC
Hmm. I do fear that we still need another QA iteration step. Sorry for that.
Comment 64 Alex Buckley 2021-08-10 22:34:28 UTC
(In reply to Marcel de Rooy from comment #63)
> Hmm. I do fear that we still need another QA iteration step. Sorry for that.

Thanks for your QA work Marcel. I will attempt to keep my responses fast, but will probably not be able to attach amended patches till the end of this week/early next week.
Comment 65 Alex Buckley 2021-08-23 06:40:03 UTC
Created attachment 124010 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

* Fix addressing comment #59 - Making can_be_deleted() consistent to
testing Patrons::find

* Fix addressing comment comment #61 - Have made the first and second
lines consistent, both now return 0

Sponsord-By: Catalyst IT
Comment 66 Alex Buckley 2021-08-23 06:52:00 UTC
Created attachment 124011 [details]
Public list with "Allow changes to contents from=Anyone seeing this list" shows to anonymous patron in Add to a list page
Comment 67 Alex Buckley 2021-08-23 06:53:49 UTC
(In reply to Marcel de Rooy from comment #63)

Hi Marcel, 

* Follow-up patch attached containing fixes for comments #59 and #61. Could you please take a look?

* Regarding comment #60: I've been testing this today. 

When a 'staff only' list is created both the virtualshelves.allow_change_from_owner and virtualshelves.allow_change_from_staff values are set to 1 (see below):

| allow_change_from_owner | allow_change_from_others | allow_change_from_staff |
+-------------------------+--------------------------+-------------------------+
|                       1 |                        0 |                       1 |
+-------------------------+--------------------------+-------------------------+

So the first condition ($self->owner == $borrowernumber &&
> $self->allow_change_from_owner) is true when a staff member who owns a 'staff only' list  and doesn't have the staff_only_lists permission tries to add/delete an item from their list. i.e. they can add/remove an item in their cart.

Am I perhaps missing something?

* Regarding comment #62: When you refer to anonymous user do you mean logging in as the borrower set in the  AnonymousPatron  system preference?

When I log into the OPAC as borrower 53 (AnonymousPatron = 53), do a search, click 'Save to lists' under a biblio, then public lists set to "Allow changes to contents from=Anyone seeing this list" show in the "Select a list" dropdown (I've attached a screenshot showing 'public' displaying in the Add to a list page), I can then add the item to that list.

Thanks,
Alex
Comment 68 Marcel de Rooy 2021-08-27 06:18:49 UTC
Will follow-up on this report after it has been signed off again.
Comment 69 Tomás Cohen Arazi 2021-09-08 11:05:31 UTC
Shouldn't we have a single 'allowed_to_change' column as an ENUM('owner','everyone','staff') ?
Comment 70 Alex Buckley 2021-09-16 21:33:44 UTC
(In reply to Tomás Cohen Arazi from comment #69)
> Shouldn't we have a single 'allowed_to_change' column as an
> ENUM('owner','everyone','staff') ?

Hi Tomas, 

Yes, I can certainly implement that. 

Thanks for testing Caroline, I will also rebase the patches against master, and re-attach them to this bug report.
Comment 71 Alex Buckley 2021-09-28 21:05:10 UTC
Created attachment 125436 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

* Fix addressing comment #59 - Making can_be_deleted() consistent to
testing Patrons::find

* Fix addressing comment comment #61 - Have made the first and second
lines consistent, both now return 0

Sponsord-By: Catalyst IT
Comment 72 Alex Buckley 2021-09-28 23:25:10 UTC
Rebased the last patch against master. Whole patchset applies cleanly now. 

I'm working on adding a single 'allowed_to_change' column as suggested by Tomás.

I think the 'allowed_to_change' column should have four values ENUM('nobody', 'owner','everyone','staff')?

This is because 'Nobody' is an option available in the 'Allow changes to contents from' dropdown when creating a list.
Comment 73 Alex Buckley 2021-09-29 22:49:23 UTC
(In reply to Tomás Cohen Arazi from comment #69)
> Shouldn't we have a single 'allowed_to_change' column as an
> ENUM('owner','everyone','staff') ?

Hi Tomás,

I've had more of a think about this, and discussed with Chris C and Aleisha. Here are our thoughts:

1. Introducing a single 'allowed_to_change' column should be done on a different bug report. This bug report is adding an enhancement. Introducing a single 'allowed_to_change' column would require existing permission logic for 'owners' and 'everyone' to be altered which is outside the scope of this enhancement. 

2. As we understand it currently a list can have a combination of permissions, hence why multiple columns were originally introduced. See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18228#c2

Furthermore in comment #67 I noted a 'staff only' list has both allow_change_from_owner=1 and allow_change_from_staff=1. 

Merging these separate columns into a single 'allowed_to_change' column would eliminate the granularity that's currently available, so we wonder if it's not the best solution.

All of that being the case, I'm marking this patchset as 'Needs signoff', as it's been rebased. Happy to hear your thoughts though!

Many thanks
Alex
Comment 74 Tomás Cohen Arazi 2021-09-29 23:09:37 UTC
(In reply to Alex Buckley from comment #73)
> (In reply to Tomás Cohen Arazi from comment #69)
> > Shouldn't we have a single 'allowed_to_change' column as an
> > ENUM('owner','everyone','staff') ?
> 
> Hi Tomás,
> 
> I've had more of a think about this, and discussed with Chris C and Aleisha.
> Here are our thoughts:
> 
> 1. Introducing a single 'allowed_to_change' column should be done on a
> different bug report. This bug report is adding an enhancement. Introducing
> a single 'allowed_to_change' column would require existing permission logic
> for 'owners' and 'everyone' to be altered which is outside the scope of this
> enhancement. 
> 
> 2. As we understand it currently a list can have a combination of
> permissions, hence why multiple columns were originally introduced. See
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18228#c2
> 
> Furthermore in comment #67 I noted a 'staff only' list has both
> allow_change_from_owner=1 and allow_change_from_staff=1. 
> 
> Merging these separate columns into a single 'allowed_to_change' column
> would eliminate the granularity that's currently available, so we wonder if
> it's not the best solution.
> 
> All of that being the case, I'm marking this patchset as 'Needs signoff', as
> it's been rebased. Happy to hear your thoughts though!

Sounds like a good review of the current status of the feature. And makes sense. I will poke at it tomorrow. My 'fear' is we could be adding some technical debt.
As I said, I'll be happy to review this.
Comment 75 Alex Buckley 2021-09-30 06:28:41 UTC
Created attachment 125472 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT
Comment 76 Alex Buckley 2021-09-30 06:31:36 UTC
(In reply to Tomás Cohen Arazi from comment #74)

> Sounds like a good review of the current status of the feature. And makes
> sense. I will poke at it tomorrow. My 'fear' is we could be adding some
> technical debt.
> As I said, I'll be happy to review this.

Thanks Tomás. Let me know how you go with your investigation tomorrow.

I've just replaced the last patch on this bug report as I realised it was missing some earlier fixes.

Ready for testing again.
Comment 77 Marcel de Rooy 2021-09-30 06:36:38 UTC
(In reply to Tomás Cohen Arazi from comment #74)
> Sounds like a good review of the current status of the feature. And makes
> sense. I will poke at it tomorrow. My 'fear' is we could be adding some
> technical debt.
> As I said, I'll be happy to review this.

Go ahead and sign off. I will resume QA after that.
Comment 78 Caroline Cyr La Rose 2021-10-06 20:47:38 UTC
*** Bug 28791 has been marked as a duplicate of this bug. ***
Comment 79 Caroline Cyr La Rose 2021-10-07 12:40:02 UTC
I waited too long to test it... I'm sorry Alex! :(
Comment 80 Alex Buckley 2021-10-07 21:10:46 UTC
(In reply to Caroline Cyr La Rose from comment #79)
> I waited too long to test it... I'm sorry Alex! :(

Hi Caroline,

Not a problem! I've rebased and will re-attach the patches.

Alex
Comment 81 Alex Buckley 2021-10-07 21:12:37 UTC
Created attachment 125914 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 82 Alex Buckley 2021-10-07 21:13:20 UTC
Created attachment 125915 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 83 Alex Buckley 2021-10-07 21:15:27 UTC
Created attachment 125916 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Alex Buckley 2021-10-07 21:15:56 UTC
Created attachment 125917 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT
Comment 85 Alex Buckley 2021-10-07 21:18:10 UTC
(In reply to Alex Buckley from comment #80)
> (In reply to Caroline Cyr La Rose from comment #79)
> > I waited too long to test it... I'm sorry Alex! :(
> 
> Hi Caroline,
> 
> Not a problem! I've rebased and will re-attach the patches.
> 
> Alex

Ready to test again Caroline :)
Comment 86 Caroline Cyr La Rose 2021-10-08 15:14:17 UTC
Hi Alex,

Patches apply well! I'm having problems with the test plan, however.

I'm stuck at step 6 from Comment #81.

When I log in with a user who did not create the list (whatever their permissions, whatever the 'changes allowed from' value (owner only or staff only)), I can't see public lists.

Here's what I tried :

1. With superlibrarian, I created a public list set to 'owner only'
   --> Public lists appear OK
2. With staff account who has 'catalog' permission, I went to Lists > Public lists
   --> Public lists appear OK
3. I applied 3 first patches, updated db (I don't have plack/memcached installed)
4. With superlibrarian, I went to Lists > Public lists
   --> Public lists appear OK
5. With staff account who has 'catalog' permission, I went to Lists > Public lists
   --> 'Processing' appears and lists never load
6. With staff account who has 'catalog' permission, I created a public list set to 'owner only'
   --> Public lists still don't appear
7. With superlibrarian, I went to Lists > Public lists
   --> Public lists don't appear anymore for this user

I also tried by applying all patches and with a staff account who has 'lists' permission and with public lists with different 'changed allowed from' values. When patches are applied, public lists appear only to the owner, only if they are all created by that same owner. If there are public lists by multiple owners, nobody can see public lists.

Is this because I don't have plack/memcached? I have a git install on my computer that I constantly update (I don't use devbox or ktd).

Caroline
Comment 87 Alex Buckley 2021-10-10 20:33:39 UTC
(In reply to Caroline Cyr La Rose from comment #86)
> Hi Alex,
> 
> Patches apply well! I'm having problems with the test plan, however.
> 
> I'm stuck at step 6 from Comment #81.
> 
> When I log in with a user who did not create the list (whatever their
> permissions, whatever the 'changes allowed from' value (owner only or staff
> only)), I can't see public lists.
> 
> Here's what I tried :
> 
> 1. With superlibrarian, I created a public list set to 'owner only'
>    --> Public lists appear OK
> 2. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> Public lists appear OK
> 3. I applied 3 first patches, updated db (I don't have plack/memcached
> installed)
> 4. With superlibrarian, I went to Lists > Public lists
>    --> Public lists appear OK
> 5. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> 'Processing' appears and lists never load
> 6. With staff account who has 'catalog' permission, I created a public list
> set to 'owner only'
>    --> Public lists still don't appear
> 7. With superlibrarian, I went to Lists > Public lists
>    --> Public lists don't appear anymore for this user
> 
> I also tried by applying all patches and with a staff account who has
> 'lists' permission and with public lists with different 'changed allowed
> from' values. When patches are applied, public lists appear only to the
> owner, only if they are all created by that same owner. If there are public
> lists by multiple owners, nobody can see public lists.
> 
> Is this because I don't have plack/memcached? I have a git install on my
> computer that I constantly update (I don't use devbox or ktd).
> 
> Caroline

Hi Caroline, 

Thank you for testing. I'll look into what might be happening here and get back to you.

Thanks!
Alex
Comment 88 Alex Buckley 2021-10-21 03:50:02 UTC
(In reply to Caroline Cyr La Rose from comment #86)
> Hi Alex,
> 
> Patches apply well! I'm having problems with the test plan, however.
> 
> I'm stuck at step 6 from Comment #81.
> 
> When I log in with a user who did not create the list (whatever their
> permissions, whatever the 'changes allowed from' value (owner only or staff
> only)), I can't see public lists.
> 
> Here's what I tried :
> 
> 1. With superlibrarian, I created a public list set to 'owner only'
>    --> Public lists appear OK
> 2. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> Public lists appear OK
> 3. I applied 3 first patches, updated db (I don't have plack/memcached
> installed)
> 4. With superlibrarian, I went to Lists > Public lists
>    --> Public lists appear OK
> 5. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> 'Processing' appears and lists never load
> 6. With staff account who has 'catalog' permission, I created a public list
> set to 'owner only'
>    --> Public lists still don't appear
> 7. With superlibrarian, I went to Lists > Public lists
>    --> Public lists don't appear anymore for this user
> 
> I also tried by applying all patches and with a staff account who has
> 'lists' permission and with public lists with different 'changed allowed
> from' values. When patches are applied, public lists appear only to the
> owner, only if they are all created by that same owner. If there are public
> lists by multiple owners, nobody can see public lists.
> 
> Is this because I don't have plack/memcached? I have a git install on my
> computer that I constantly update (I don't use devbox or ktd).
> 
> Caroline

Hi Caroline, 

Thanks very much for testing. I've been trying to replicate (I use kohadevbox) but unfortunately I haven't been able to so I have a few questions:

Q1. Could you please let me know are the tests you did above in the staff client or the OPAC?

Q2. In step 5,6, and 7 if you right click, select 'Inspect element' then look in the 'Console' tab do you see any errors?

Q3. If you try applying the patches in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) do you also still encounter this problem?

Thanks
Alex
Comment 89 Blou 2021-10-21 18:15:50 UTC
This patch would be very helpful to some of our customers.  But what would it take to extend the concept to private lists to be viewable by staff ?

There's Bug 19501 opened for that, but it seems to me this patch here is almost there already.
Comment 90 Alex Buckley 2021-11-12 02:46:39 UTC
Created attachment 127556 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT
Comment 91 Alex Buckley 2021-11-12 02:47:03 UTC
Created attachment 127557 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 92 Alex Buckley 2021-11-12 02:47:25 UTC
Created attachment 127558 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 93 Alex Buckley 2021-11-12 02:54:52 UTC
Created attachment 127559 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 94 Alex Buckley 2021-11-12 02:55:17 UTC
Created attachment 127560 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT
Comment 95 Alex Buckley 2021-11-12 03:28:07 UTC
Hi Caroline,

I've rebased this patchset against master. Would you mind testing this patchset in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) to see if you still encounter the problems in comment #86?

---

(In reply to Blou from comment #89)
> This patch would be very helpful to some of our customers.  But what would
> it take to extend the concept to private lists to be viewable by staff ?
> 
> There's Bug 19501 opened for that, but it seems to me this patch here is
> almost there already.

Hi Blou, 

Apologies for my slow response! My team has been flat out with client work recently.

I'll have a think about this and try to get back to you as soon as possible.

Thanks,
Alex
Comment 96 Alex Buckley 2021-11-25 20:45:00 UTC
Created attachment 128017 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table

Sponsored-By: Catalyst IT
Comment 97 Alex Buckley 2021-11-25 20:46:42 UTC
Created attachment 128018 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT
Comment 98 Alex Buckley 2021-11-25 20:47:18 UTC
Created attachment 128019 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 99 Alex Buckley 2021-11-25 20:52:05 UTC
Created attachment 128020 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 100 Alex Buckley 2021-11-25 20:52:31 UTC
Created attachment 128021 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 101 Alex Buckley 2021-11-25 20:52:56 UTC
Created attachment 128022 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT
Comment 102 Alex Buckley 2021-11-25 21:01:33 UTC
Created attachment 128023 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT
Comment 103 Alex Buckley 2021-11-25 21:10:32 UTC
I've uploaded a fixed second patch on this bug report which was causing the t/db_dependent/Virtualshelves.t unit test to fail.
Comment 104 Alex Buckley 2021-11-25 22:38:29 UTC
(In reply to Alex Buckley from comment #95)
> Hi Caroline,
> 
> I've rebased this patchset against master. Would you mind testing this
> patchset in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) to
> see if you still encounter the problems in comment #86?
> 
> ---
> 
> (In reply to Blou from comment #89)
> > This patch would be very helpful to some of our customers.  But what would
> > it take to extend the concept to private lists to be viewable by staff ?
> > 
> > There's Bug 19501 opened for that, but it seems to me this patch here is
> > almost there already.
> 
> Hi Blou, 
> 
> Apologies for my slow response! My team has been flat out with client work
> recently.
> 
> I'll have a think about this and try to get back to you as soon as possible.
> 
> Thanks,
> Alex

Hi Blou, 

Yes, extending this patchset to implement functionality requested in Bug 19501 would be straightforward except for the fact that the virtualshelves.category has been changed to a boolean virtualshelves.public field in bug 28959. 
Meaning adding a third option of 'Restricted', or 'Staff client only' in that field, would be regressing 28959.

However, the way I would approach the functionality you're after is:

* Change virtualshelves.public back to virtualshelves.category
* Add a new option to virtualshelves.category of 3 (Staff client only)
* Add two new patron lists sub-permissions:
- 'Delete staff client lists (delete_staff_client_lists)'
- 'Edit staff client lists (edit_staff_client_lists)'

When a 'staff client only' list is created it would be viewable by all logged-in users on the staff client. It would not be visible to anyone in the OPAC. 

Only users with the 'edit_staff_client_lists' sub-permission would be able to edit it. Similarly, it would only be deletable by users with the 'delete_staff_client_lists' sub-permission.

Thanks!
Alex
Comment 105 Alex Buckley 2021-12-10 04:06:18 UTC
Hi all, 

I've rebased this patchset, added a missing change to kohastructure.sql, and merged the following 4 patches into 1 patch:

-  Bug 26346: Add option to make public lists editable by all staff
-  Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists
-  Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user
-  Bug 26346: (follow-up) Fixes in response to QA

I have also reworked the test plan to hopefully make it more succinct, and easier to follow.

Alex
Comment 106 Alex Buckley 2021-12-10 04:08:20 UTC
Created attachment 128383 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT
Comment 107 Alex Buckley 2021-12-10 04:08:58 UTC
Created attachment 128384 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT
Comment 108 Alex Buckley 2021-12-10 04:09:26 UTC
Created attachment 128385 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 109 Alex Buckley 2021-12-10 04:58:26 UTC
Ready to test again!
Comment 110 Shi Yao Wang 2022-03-10 15:58:12 UTC
Here is the result after I tried to apply the changes with `git bz apply 26346`:


Bug 26346 - Add option to make a public list editable by library staff only

128383 - Bug 26346: Database changes
128384 - Bug 26346: Updated schema file
128385 - Bug 26346: Add option to make public lists editable by all staff

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26346: Database changes
Applying: Bug 26346: Updated schema file
Applying: Bug 26346: Add option to make public lists editable by all staff
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
M	opac/opac-shelves.pl
M	t/db_dependent/Koha/Patron.t
M	virtualshelves/shelves.pl
Falling back to patching base and 3-way merge...
Auto-merging virtualshelves/shelves.pl
Auto-merging t/db_dependent/Koha/Patron.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patron.t
Auto-merging opac/opac-shelves.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26346: Add option to make public lists editable by all staff
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26346-Add-option-to-make-public-lists-editable-HTuW5n.patch
Comment 111 Alex Buckley 2022-03-14 23:09:42 UTC
(In reply to shi-yao.wang from comment #110)

Thanks, Shi-Yao I will rebase my patches against master and re-attach them to this ticket to re-test.

Thanks,
Alex
Comment 112 Alex Buckley 2022-03-15 00:41:13 UTC
Created attachment 131699 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 113 Alex Buckley 2022-03-15 00:44:04 UTC
Created attachment 131700 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 114 Alex Buckley 2022-03-15 00:44:29 UTC
Rebased and ready to test again!
Comment 115 Shi Yao Wang 2022-03-15 13:25:19 UTC
Patch doesn't apply.



Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26346: Database changes
Applying: Bug 26346: Updated schema file
Applying: Bug 26346: Add option to make public lists editable by all staff
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
M	t/db_dependent/Koha/Patron.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patron.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26346: Add option to make public lists editable by all staff
Comment 116 Blou 2022-03-15 18:07:48 UTC
Created attachment 131757 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 117 Blou 2022-03-15 18:08:34 UTC
I fixed the patch.
Comment 118 Shi Yao Wang 2022-03-15 18:47:40 UTC
At step 6, when logged in the staff client as User B, if I go to Lists -> Public lists it loads the page forever so I cannot access the individual public list page.
Comment 119 Alex Buckley 2022-03-16 03:49:30 UTC
Created attachment 131764 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 120 Alex Buckley 2022-03-16 03:52:57 UTC
Thanks for rebasing Blou! I've just attached the final patch again as the t/db_dependent/Koha/Patron.t unit test was failing. 

Shi-yao, I've gone through the test plan on my master branch with the patches applied, but unfortunately I am not encountering what you saw in step 6. 

Can you please paste what you see in the plack-error.log file?
tail -f /var/log/koha/<instancename>/plack-error.log

Blou or Andrew, could either of you please see if you're able to replicate what Shi-yao is seeing on step 6?
Comment 121 Owen Leonard 2022-03-16 11:42:23 UTC
(In reply to Alex Buckley from comment #120)

> Blou or Andrew, could either of you please see if you're able to replicate
> what Shi-yao is seeing on step 6?

I can't reproduce that problem. The page loads correctly for me under those circumstances. However, I see a couple of complaints from the QA tool that should be checked.
Comment 122 Shi Yao Wang 2022-03-16 16:05:57 UTC
I have no output from my plack file, but since nobody has this problem it should only be an issue on my own setup.
Comment 123 Alex Buckley 2022-03-16 21:25:16 UTC
(In reply to Owen Leonard from comment #121)
> (In reply to Alex Buckley from comment #120)
> 
> > Blou or Andrew, could either of you please see if you're able to replicate
> > what Shi-yao is seeing on step 6?
> 
> I can't reproduce that problem. The page loads correctly for me under those
> circumstances. However, I see a couple of complaints from the QA tool that
> should be checked.

Hi Owen, 

Thanks very much for testing!

Could you please paste the output from your QA tool? 

When I run the QA tool the output I get looks fine (I think): https://paste.koha-community.org/8282

Thanks,
Alex
Comment 124 Alex Buckley 2022-03-28 00:57:51 UTC
(In reply to Alex Buckley from comment #123)
> (In reply to Owen Leonard from comment #121)
> > (In reply to Alex Buckley from comment #120)
> > 
> > > Blou or Andrew, could either of you please see if you're able to replicate
> > > what Shi-yao is seeing on step 6?
> > 
> > I can't reproduce that problem. The page loads correctly for me under those
> > circumstances. However, I see a couple of complaints from the QA tool that
> > should be checked.
> 
> Hi Owen, 
> 
> Thanks very much for testing!
> 
> Could you please paste the output from your QA tool? 
> 
> When I run the QA tool the output I get looks fine (I think):
> https://paste.koha-community.org/8282
> 
> Thanks,
> Alex

Hi Marcel and all, 

Would you mind pasting the content of your QA test tool as when I run my tool I don't get any failures?

Many thanks,
Alex
Comment 125 Alex Buckley 2022-03-28 00:59:57 UTC
(In reply to Alex Buckley from comment #124)
> (In reply to Alex Buckley from comment #123)
> > (In reply to Owen Leonard from comment #121)
> > > (In reply to Alex Buckley from comment #120)
> > > 
> > > > Blou or Andrew, could either of you please see if you're able to replicate
> > > > what Shi-yao is seeing on step 6?
> > > 
> > > I can't reproduce that problem. The page loads correctly for me under those
> > > circumstances. However, I see a couple of complaints from the QA tool that
> > > should be checked.
> > 
> > Hi Owen, 
> > 
> > Thanks very much for testing!
> > 
> > Could you please paste the output from your QA tool? 
> > 
> > When I run the QA tool the output I get looks fine (I think):
> > https://paste.koha-community.org/8282
> > 
> > Thanks,
> > Alex
> 
> Hi Marcel and all, 
> 
> Would you mind pasting the content of your QA test tool as when I run my
> tool I don't get any failures?
> 
> Many thanks,
> Alex

Correction: Sorry this should be: Hi Owen and all
Comment 126 Katrin Fischer 2022-03-28 21:38:17 UTC
Hi Alex, these are the results when I run the QA tools:

Processing files before patches
|========================>| 13 / 13 (100.00%)
Processing files after patches
|========================>| 13 / 13 (100.00%)

 OK	Koha/Patron.pm
 OK	Koha/Schema/Result/Virtualshelve.pm
 OK	Koha/Virtualshelf.pm
 OK	Koha/Virtualshelves.pm
 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  tinyint_has_boolean_flag
		The new column virtualshelves.public is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
		The new column virtualshelves.allow_change_from_staff is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
 OK	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
 OK	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
 OK	opac/opac-addbybiblionumber.pl
 OK	opac/opac-shelves.pl
 OK	t/db_dependent/Koha/Patron.t
 FAIL	t/db_dependent/Virtualshelves.t
   FAIL	  spelling
		 thier ==> their

 OK	virtualshelves/shelves.pl
Comment 127 Alex Buckley 2022-03-28 21:41:30 UTC
Hi Katrin, 

Thank you very much, I'll get onto fixing this right away!

Thanks,
Alex
Comment 128 Alex Buckley 2022-03-28 22:12:08 UTC
Created attachment 132373 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand
Comment 129 Alex Buckley 2022-03-28 22:13:39 UTC
(In reply to Katrin Fischer from comment #126)
> Hi Alex, these are the results when I run the QA tools:
> 
> Processing files before patches
> |========================>| 13 / 13 (100.00%)
> Processing files after patches
> |========================>| 13 / 13 (100.00%)
> 
>  OK	Koha/Patron.pm
>  OK	Koha/Schema/Result/Virtualshelve.pm
>  OK	Koha/Virtualshelf.pm
>  OK	Koha/Virtualshelves.pm
>  FAIL	installer/data/mysql/kohastructure.sql
>    FAIL	  tinyint_has_boolean_flag
> 		The new column virtualshelves.public is using TINYINT(1) as type but is
> not defined as boolean in the schema file
> (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
> 		The new column virtualshelves.allow_change_from_staff is using TINYINT(1)
> as type but is not defined as boolean in the schema file
> (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
> 
>  OK	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
>  OK	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
>  OK	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
>  OK	opac/opac-addbybiblionumber.pl
>  OK	opac/opac-shelves.pl
>  OK	t/db_dependent/Koha/Patron.t
>  FAIL	t/db_dependent/Virtualshelves.t
>    FAIL	  spelling
> 		 thier ==> their
> 
>  OK	virtualshelves/shelves.pl

Hi Katrin, 

Could you please re-run the QA test tool now after applying the fourth patch on this bug report? 
Hopefully, that will fix up those QA errors. 

Many thanks,
Alex
Comment 130 Lucas Gass 2022-03-29 16:22:16 UTC
Created attachment 132475 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 131 Lucas Gass 2022-03-29 16:22:22 UTC
Created attachment 132476 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 132 Lucas Gass 2022-03-29 16:22:30 UTC
Created attachment 132477 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 133 Lucas Gass 2022-03-29 16:22:36 UTC
Created attachment 132478 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 134 Lucas Gass 2022-03-29 16:24:51 UTC
Created attachment 132479 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 135 Lucas Gass 2022-03-29 16:26:43 UTC
This all seems to work great! Followed the test plan and everything worked as described. 

Tests passed and QA script seems to be happy now! This is a great addition.
Comment 136 Alex Buckley 2022-03-29 23:35:26 UTC
(In reply to Lucas Gass from comment #135)
> This all seems to work great! Followed the test plan and everything worked
> as described. 
> 
> Tests passed and QA script seems to be happy now! This is a great addition.

Thank you Lucas!
Comment 137 Nick Clemens 2022-03-31 13:14:21 UTC
Created attachment 132751 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 138 Nick Clemens 2022-03-31 13:14:29 UTC
Created attachment 132752 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 139 Nick Clemens 2022-03-31 13:14:39 UTC
Created attachment 132753 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 140 Nick Clemens 2022-03-31 13:15:01 UTC
Created attachment 132754 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 141 Nick Clemens 2022-03-31 13:15:12 UTC
Created attachment 132755 [details] [review]
Bug 26346: (QA follow-up) Clarify permission

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 142 Nick Clemens 2022-03-31 13:19:56 UTC
Tested, this works as described and passes tests, great work Alex.

I admit to being confused at first on what the permission did, so I added a follow-up to clarify, let me know if it makes sense.

I also filed bug 30418 for the functionality I thought the permission added.
Comment 143 Alex Buckley 2022-03-31 20:21:44 UTC
(In reply to Nick Clemens from comment #142)
> Tested, this works as described and passes tests, great work Alex.
> 
> I admit to being confused at first on what the permission did, so I added a
> follow-up to clarify, let me know if it makes sense.
> 
> I also filed bug 30418 for the functionality I thought the permission added.

Hi Nick, 

Yay, thanks so much! 

Yes, I think your follow-up clarifies the edit_public_lists permission nicely.

Many thanks,
Alex
Comment 144 Fridolin Somers 2022-04-12 15:14:08 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 145 Marcel de Rooy 2022-06-08 11:37:24 UTC
Not sure if it is coming from here, but it should be the first report to blame :)

If I am adding an entry to a new list on OPAC, and I select Public list, it adds the entry to a private list.

This is rather confusing?
Comment 146 Marcel de Rooy 2022-06-08 11:51:47 UTC
Opened bug 30925
Comment 147 Marcel de Rooy 2022-06-08 12:10:07 UTC
(In reply to Marcel de Rooy from comment #145)
> Not sure if it is coming from here, but it should be the first report to
> blame :)

Might be bug 28959 btw