Bug 34610

Created attachment 169633 [details] [review]
Bug 34610: Udate deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers and permissions permissions to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

I tried to get the hover message with the incorrect text ("The system preference ProtectSuperlibrarian is enabled") to display before applying the patch, but I wasn't able to. (I normally do this so that I can show that the patch fixed the issue.)

I think something else is going on here, and I don't think the ProtectSuperlibrarianPrivileges system preference is working as it should.

So I will leave for someone else to test that has a better understanding of the code and how permissions work.

A couple of other notes:
1. The commit message title needs updating "..Udate.." to "...Update...".
2. To get credit on the dashboard (https://dashboard.koha-community.org/), and to be listed as a Koha developer on the Koha timeline page, please update the assignee for the bug.

How I tested
============

This is what I did, with and without the patch applied, as per the test plan (using koha-testing docker (KTD) and the sample data already set up):
1. Set the ProtectSuperlibrarianPrivileges system preference to "Allow only superlibrarians",
2. Set up permissions to test: [1]
   - User 1 = Henry Acevedo (permissions: superlibrarian permission)
   - User 2 = Mary Burton (permissions: catalogue, borrowers, and permissions), change password so you can log in as Mary
   - User 3 = Edna Acosta (permissions: catalogue), so can test anotehr staff patron with minimal access
3. If I login as User 2 (Mary) and go to edit the permissions for User 1 (Henry), I get the login form with the message "Error: You do not have permission to access this page."
  . This seems correct, Mary has the permission permission, but is not a superlibrarian. As ProtectSuperlibrarianPrivileges is set to "[Allow only superlibrarians] to access/change superlibrarian privileges." (step 1 of the test plan), she should not be able to change the permissions for superlibrarians. So she can't view the permissions page (so can't change them).
  . Maybe another bug is preventing User 2 (Mary) from viewing User 1's (Henry's) permissions?
  . Maybe it should be showing the permissions, but not them be changed and display the incorrect hover message?
4. If I repeat step 3, but try to add super librarian permissions to a staff patron who is not a a super librarian (User 3, Edna Acosta), I get the same error as I got for Henery in step 3 above.
5. If I try to set the permissions for another non-staff patron (I chose Lisa Charles, ), I get the hover message but it has the correct system preference name.
6. If I repeat steps 3 to 5, with ProtectSuperlibrarianPrivileges set to "Allow all permitted users", Mary should be able to change the permissions for superlibrarians and add superlibrarian privileges to other patrons:
   . Mary trying to change Henry's permissions: can't change, get "Error: You do not have permission to access this page."
   . Mary trying to change Edna's permissions: can't change get "Error: You do not have permission to access this page."
   . Mary trying to change Lisa's permissions (K - Kid patron category): I can change to a superlibrarian!
7. I applied the patch and repeated the steps - I got the same results.

NOTES: 
1. I logged Mary out each time I changed any permissions or system preference values.
2. If I applied the patch, and did a restart_all, for step 4 above, I get the login form and "Error: You do not have permission to access this page.".

[1] Permission names and codes
------------------------------

- Access to all librarian functions (superlibrarian)
- Staff access, allows viewing of catalogue in staff interface (catalogue)
- Add, modify and view patron information (borrowers)
- Set user permissions (permissions)

Created attachment 170201 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

(In reply to David Nind from comment #2)
>    - User 2 = Mary Burton (permissions: catalogue, borrowers, and
> permissions)

I think the test plan was incorrect and I have modified it: I think the testing process assumes that the user also has the 'staffaccess' permission: "Allow staff members to modify permissions, usernames, and passwords for other staff members."

In any case the patch is correct and if there is another issue with this permission we can file a new bug.

Created attachment 170321 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Created attachment 170322 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the Select all and Clear all links.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

I made one more edit to the test plan - the deprecated string was in the tooltip message for the Select all / Clear all buttons, not the superlibrarian permissions checkbox. Otherwise, everything looks good! Passing QA

I've updated the author line in the patch to:
Author: Noémie Ariste <noemie.ariste@catalyst.net.nz>

Thanks Noémie and welcome as a new Koha developer!

Created attachment 170455 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the Select all and Clear all links.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Pushed for 24.11!

Well done everyone, thank you!