|
Description
Lucas Gass (lukeg)
2022-04-26 20:18:14 UTC
Created attachment 136055 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Realized this needs a few tweaks before it is ready for testing. Setting to assigned. Created attachment 136076 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Hi Lucas, i believe the plan changed - could you please update the bug description? This patch attempts to decouple the ability to change branches from the system preference IndependentBranches. Instead of having to turn on IndependentBranches, which does a bunch of other things, libraries can now use a permission (select_loggedin_library) to control who can change branches. Lucas demonstrated the test plan and I'm signing off on the work - Bob Bennhoff Created attachment 137733 [details] [review] Bug 30624: DB Update to add permission Created attachment 138377 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 138378 [details] [review] Bug 30624: DB Update to add permission Signed-off-by: Andrew <andrewfh@gmail.com> We must also impact the staff authentication page no ? (In reply to Fridolin Somers from comment #10) > We must also impact the staff authentication page no ? Unless I am mistaken by the question, that is handled in the first patch. ( Auth.pm ) I mean in authentication TT. Actually there is only a UNLESS IndependentBranches : https://git.koha-community.org/Koha-community/Koha/src/commit/fc78953951837f6fe7a3e9d66ac497dcb5511f53/koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt#L91 Created attachment 139961 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 139962 [details] [review] Bug 30624: DB Update to add permission Signed-off-by: Andrew <andrewfh@gmail.com> rebased for current master (In reply to Fridolin Somers from comment #12) > I mean in authentication TT. > Actually there is only a UNLESS IndependentBranches : > > https://git.koha-community.org/Koha-community/Koha/src/commit/ > fc78953951837f6fe7a3e9d66ac497dcb5511f53/koha-tmpl/intranet-tmpl/prog/en/ > modules/auth.tt#L91 Ah, yes! Since that will be handled by a permission we no longer need this check, the dropdown should always show. Created attachment 139963 [details] [review] Bug 30624: (follow-up) Remove IndependentBraches check on auth.tt No longer applies to current master, rebase coming. Created attachment 141069 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 141070 [details] [review] Bug 30624: DB Update to add permission Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 141071 [details] [review] Bug 30624: (follow-up) Remove IndependentBraches check on auth.tt There is a change on set-library.tt causing a conflict that I am not sure enough about to resolve. Can you please rebase? Created attachment 142269 [details] [review] Bug 30624: Add select_loggedin_library permission To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new permission 'select_loggedin_library' on. 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'select_loggedin_library' back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 142270 [details] [review] Bug 30624: DB Update to add permission Signed-off-by: Andrew <andrewfh@gmail.com> Created attachment 142271 [details] [review] Bug 30624: (follow-up) Remove IndependentBraches check on auth.tt Rebased for current master Back here. There are several remaining issues here :(
1) Database update
The database update takes care of adding the permission, but it doesn't take care to update the existing staff accounts. With this patch as is people would loose the ability to change libraries on update which would be a major behavior change.
As you remove IndependentBranches checks, we should factor this in as well. I guess if IndependentBranches is used, don't update non-superlibrarians. If it isn't used, update non-superlibrarians with the new permission.
2) Unit test
The change to Auth.pm requires a unit test
3) The permission check in Auth.pm doesn't look quite right... maybe instead of $flags->{superlibrarian} $patron->is_superlibrarian should be used?
4) Why do you remove the check in koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt in auth.tt without replacing it with a permission check?
5) Permission
I am not sure the permission is a good fit for the "Administration" module. So far, we use this for access to the different parts of the administration start page only. It feels more at the same level as staff_access maybe?
6) Login page staff
The page still offers the library pull down, as of course, the permissions can be different by user. But maybe we should give a warning if someone who can't do so tries to log in with another library? (non-blocker)
7) Translatability
I believe this construct is problematic or all TT variables would show up in translations:
+[% SET page_title = "" %]
+[% IF ( CAN_user_parameters_select_loggedin_library ) %]
+ [% IF Koha.Preference('UseCirculationDesks') %]
+ [% page_title = "Set library and desk" %]
+ [% ELSE %]
+ [% page_title = "Set library" %]
+ [% END %]
+[% ELSE %]
+ [% IF Koha.Preference('UseCirculationDesks') %]
+ [% page_title = "Set desk"%]
+ [% END %]
+[% END %]
> 5) Permission > > I am not sure the permission is a good fit for the "Administration" module. > So far, we use this for access to the different parts of the administration > start page only. It feels more at the same level as staff_access maybe? Thanks for taking the time to look through this Cait! To clarify, do you think 'select_loggedin_library' subpermisson should go under "Allow staff members to modify permissions, usernames, and passwords for other staff members (staffaccess)"? As I started playing around with this I noticed another possible bug, Bug 32036. Hi Lucas, I think you need at least 2 sub-permissions then, that's why it's not working. Like: new thing X and remaining. What I meant is that I see it more as a top level or maybe as a sub-permission of a new module as it doesn't fit with the rest... but I am not sure how to phrase it. Created attachment 144557 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update Created attachment 144559 [details] [review] Bug 30624: Unit tests To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Created attachment 144584 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Created attachment 144594 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Created attachment 144595 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Created attachment 144596 [details] [review] Bug 30624: Unit tests Created attachment 145048 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off by: Bob Bennhoff/AspenCat Team Created attachment 145049 [details] [review] Bug 30624: Unit tests Signed-off by: Bob Bennhoff/AspenCat Team Created attachment 145631 [details] [review] Bug 30624: Adjust number of tests 1) We have a mix now of $flags->{superlibrarian} == 1 and $patron->is_superlibrarian
I remember that is_superlibrarian was created in order to fix and avoid a lot of false/wrong permission checks for superlibrarian in the codebase. I am not sure if the flags will work correctly, maybe someone else could weigh in? Should we be consistent or doesn't it matter?
2) Database update
UPDATE borrowers SET flags = flags + (1<<29) WHERE flags & 4
Can you explain that one to me? I have written stuff like that before, but it's not coming back to me right now. I had expected something like "where flags != 0 and IS NOT NULL"?
3) Changing home library
The staff user can still change their own home branch and circumvent the limitation that way. Should we make the library read only when the permission is missing?
These are mostly questions, hope we can move this forward!
(In reply to Katrin Fischer from comment #40) > 1) We have a mix now of $flags->{superlibrarian} == 1 and > $patron->is_superlibrarian > > I remember that is_superlibrarian was created in order to fix and avoid a > lot of false/wrong permission checks for superlibrarian in the codebase. I > am not sure if the flags will work correctly, maybe someone else could weigh > in? Should we be consistent or doesn't it matter? The inconsistency is set-library.pl. Instead of building a patron object so that we can use $patron->is_superlibrarian I think we can use C4::Context for the check. > 2) Database update > > UPDATE borrowers SET flags = flags + (1<<29) WHERE flags & 4 > > Can you explain that one to me? I have written stuff like that before, but > it's not coming back to me right now. I had expected something like "where > flags != 0 and IS NOT NULL"? Using 1<<29 since borrower.flags is stored in bits. It's the bitwise operator '<<'. (right shift) I used in order to preserve other permissions set in borrower.flag. It takes the original value and adds (1<<29). > 3) Changing home library > > The staff user can still change their own home branch and circumvent the > limitation that way. Should we make the library read only when the > permission is missing? Good point. Do you think that is within the scope of this bug? Created attachment 147509 [details] [review] Bug 30624: (QA follow-up) Use C4:Context to get permssions In an effoft to keep this moving I am removing the 'FQA' status since I answered the questions asked and provided a follow-up. > > 2) Database update > > > > UPDATE borrowers SET flags = flags + (1<<29) WHERE flags & 4 > > > > Can you explain that one to me? I have written stuff like that before, but > > it's not coming back to me right now. I had expected something like "where > > flags != 0 and IS NOT NULL"? > > Using 1<<29 since borrower.flags is stored in bits. It's the bitwise > operator '<<'. (right shift) I used in order to preserve other permissions > set in borrower.flag. It takes the original value and adds (1<<29). I understood that part, I was wondering about the flags & 4 in the where condition :) > > > 3) Changing home library > > > > The staff user can still change their own home branch and circumvent the > > limitation that way. Should we make the library read only when the > > permission is missing? > Good point. Do you think that is within the scope of this bug? I am torn, I think it would make it feature complete, but also ok to handle as a follow-up. What do you think? > 2) Database update
>
> UPDATE borrowers SET flags = flags + (1<<29) WHERE flags & 4
>
> Can you explain that one to me? I have written stuff like that before, but
> it's not coming back to me right now. I had expected something like "where
> flags != 0 and IS NOT NULL"?
Because I want to set the new loggedinlibrary permission for patrons that have the staff access permission when IndependentBranches is turned on.
Now that I think about it what you say would work too "WHERE flags != 0 and IS NOT NULL".
Happy to change it if you'd like!
(In reply to Lucas Gass from comment #45) > > 2) Database update > > > > UPDATE borrowers SET flags = flags + (1<<29) WHERE flags & 4 > > > > Can you explain that one to me? I have written stuff like that before, but > > it's not coming back to me right now. I had expected something like "where > > flags != 0 and IS NOT NULL"? > > Because I want to set the new loggedinlibrary permission for patrons that > have the staff access permission when IndependentBranches is turned on. > > Now that I think about it what you say would work too "WHERE flags != 0 and > IS NOT NULL". > > Happy to change it if you'd like! Nick brought up a good point. We should specifically check for the staff access (catalogue) permission. Some API accounts may have other permissions but not staff access. > > > 3) Changing home library
> > >
> > > The staff user can still change their own home branch and circumvent the
> > > limitation that way. Should we make the library read only when the
> > > permission is missing?
We probably need to limit which branches we pass to the template. If someone is acting nefariously they could potentially use the browser dev tools to remove a readonly attribute.
> > 3) Changing home library
> >
> > The staff user can still change their own home branch and circumvent the
> > limitation that way. Should we make the library read only when the
> > permission is missing?
Cait, I discussed this a bit more with Nick. We don't think we need any additional work here. There is a the 'view_borrower_infos_from_any_libraries' subpermission to take care of this. Do you agree?
(In reply to Lucas Gass from comment #48) > > > 3) Changing home library > > > > > > The staff user can still change their own home branch and circumvent the > > > limitation that way. Should we make the library read only when the > > > permission is missing? > > Cait, I discussed this a bit more with Nick. We don't think we need any > additional work here. There is a the > 'view_borrower_infos_from_any_libraries' subpermission to take care of this. > Do you agree? I think we could make it a separate bug, but I don't think that 'view_borrower_infos_from_any_libraries covers it, unless it removes the ability to reset my own (staff accounts) homebranch? > I think we could make it a separate bug, but I don't think that
> 'view_borrower_infos_from_any_libraries covers it, unless it removes the
> ability to reset my own (staff accounts) homebranch?
It does. Without the view_borrower_infos_from_any_libraries a staff member cannot change their own branch or anyone else's.
(In reply to Lucas Gass from comment #50) > > I think we could make it a separate bug, but I don't think that > > 'view_borrower_infos_from_any_libraries covers it, unless it removes the > > ability to reset my own (staff accounts) homebranch? > > It does. Without the view_borrower_infos_from_any_libraries a staff member > cannot change their own branch or anyone else's. Interesting! I didn't know that. I wonder how we could advertise that better somehow. Separate issue, but something to think about. I would have seen it more as part of this permission. Maybe both should have the effect? (not blocking, just thinking loudly about possible next steps) (In reply to Lucas Gass from comment #46) > (In reply to Lucas Gass from comment #45) > > Nick brought up a good point. We should specifically check for the staff > access (catalogue) permission. Some API accounts may have other permissions > but not staff access. Makes sense, thanks! Created attachment 147753 [details] [review] Bug 30624: Add loggedinlibrary permission and DB update To test: 1. Apply patch, updatedatabase, restart_all 2. Have a user with superlibrarian privileges ( User1 ) 3. Have a user who has staff access and circulate privileges but is not a super librarian. ( User2 ) Make note of this users home library 4. Turn on the system preference 'CircSidebar'. -MAIN log in ( auth.tt ) 5. As User1, go to the main login screen and try logging in. You should be able to log in AND you should be able to properly chnage your branch BEFORE logging in. 6. As User2, to to the main login screnn amd try logging in. You should be able to but if you try and switch your libraray to anything beside the user's home branch it will not work. You will be logged in at your home branch. 7. For User2, set the new top level permission 'Allow staff to change logged in library (loggedinlibrary). 8. Now you should be able to successfully switch libraries before log in. 9. Turn the 'loggedinlibrary' permission back off for User2. -AFTER log in- 10. With User1, click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 11. With User2, click on your name/branch in the top right. If you have 'UseCirculationDesks' on, you should see 'Set desk', otherwise you should see nothing. 12. Repeat step 7. 13. NOw if you click on your name/branch in the top right, you should see the the link 'Set library' at the top. If you turn on 'UseCirculationDesks' the link will be 'Set library and desk'. 14. Repeat Step 9. -CircSideBar- 15. With 'CircSideBar' turned on, go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup) with User1. You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 16. Try with User2 and you will not see a 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set desk' link. 17. Repeat step 7. 18. For with User2 you go to any ciculation page (Holds queue, Holds to pull, Holds awaiting pickup). You will see the 'Set library' link. If 'UseCirculationDesks' is on you will see a 'Set library and desk'. 19. Repeat step 9. -Set library page- 20. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User1. You will see a dropdown for 'Set library'. Make sure you can change your library successfully. 21. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. You should NOT see see a dropdown for 'Set library'. 22. Repeat step 7. 23. Go to the set library page (http://localhost:8081/cgi-bin/koha/circ/set-library.pl) with User2. Now you should see a dropdown for 'Set library'. Signed-off by: Bob Bennhoff/AspenCat Team Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 147754 [details] [review] Bug 30624: Unit tests Signed-off by: Bob Bennhoff/AspenCat Team Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 147755 [details] [review] Bug 30624: Adjust number of tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 147756 [details] [review] Bug 30624: (QA follow-up) Use C4:Context to get permissions Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I see this error when trying to run the atomicupdate:
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Out of range value for column 'flags' at row 80 at /kohadevbox/koha/C4/Installer.pm line 739
(In reply to Tomás Cohen Arazi from comment #57) > I see this error when trying to run the atomicupdate: > > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Out of range > value for column 'flags' at row 80 at /kohadevbox/koha/C4/Installer.pm line > 739 I don't recreate this. Can you tell me how you are? (In reply to Lucas Gass from comment #58) > (In reply to Tomás Cohen Arazi from comment #57) > > I see this error when trying to run the atomicupdate: > > > > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Out of range > > value for column 'flags' at row 80 at /kohadevbox/koha/C4/Installer.pm line > > 739 > > I don't recreate this. Can you tell me how you are? I see, when permissions are set a certain way the binary becomes too big too fit in the flags column. too big* Hm. What's the best fix here? Should we extend the borrowers.flags (and deletedborrowers and borrower_modifications) on a separate bug? Ultimately we should move away from the bit structure, but maybe we could do a "quick" fix and not push a permission rewrite on this one. Created attachment 148116 [details] [review] Bug 30624: Make DB update idempotent (In reply to Katrin Fischer from comment #61) > Hm. What's the best fix here? Should we extend the borrowers.flags (and > deletedborrowers and borrower_modifications) on a separate bug? > > Ultimately we should move away from the bit structure, but maybe we could do > a "quick" fix and not push a permission rewrite on this one. I think the best fix for this bug is to make the DB update idempotent. I have added a follow-up to do so. Pushed to master for 23.05. Nice work everyone, thanks! Enhancement - not backporting to 22.11.x Nice work everyone! |