|
Description
Sam Lau
2023-07-03 15:26:19 UTC
Created attachment 153337 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected.9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Created attachment 153338 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected.9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Created attachment 153348 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected.9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Hi Sam, congrats on your patch :) Some notes: 1) The patch description should always describe the fix, not the problem. For example here: Add system preference to make library required on login 2) Always run the QA test tools: FAIL installer/data/mysql/atomicupdate/bug_34188.pl FAIL file permissions File must have the exec flag FAIL koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt FAIL filters missing_filter at line 121 ( <label for="branch" class="[% libraryClass %]">Library:</label>) missing_filter at line 122 ( <select name="branch" id="branch" class="input" tabindex="3" [% libraryRequired %]>) FAIL tt_valid lines 122 There is a script in misc/devel that can help you fix the filters quickly: add_missing_filters.pl The TT valid points to some template issue, double check your changes for this one. Overall this looks good - close! Created attachment 153563 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected. 9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Created attachment 153605 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected. 9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Signed-off-by: Owen Leonard <oleonard@myacpl.org> 1. Why Forced* instead of Force*? 2. You are not enforcing the condition server-side, shouldn't we? Agreed on the serverside requirement.. we should really check the pref there too rather than do the standard auto-selection. I love the work here though.. we do with clientside with jQuery all the time.. it would be nice to have a switch instead. (In reply to Martin Renvoize from comment #8) > Agreed on the serverside requirement.. we should really check the pref there > too rather than do the standard auto-selection. (In reply to Jonathan Druart from comment #7) > 2. You are not enforcing the condition server-side, shouldn't we? I think we did talk about this initially - your library won't be removed from the list, so is still a valid selection. If you are going to far as to trick the script to sign you in to your library, I think you have been forced to make a selection :-) We could add a check, but it would need to be in Auth.pm, and is a bigger change that need more tests. I don't see this one so much as a security feature, but a convenience feature to ensure libraries don't accidentally sign in to the wrong branch Created attachment 157285 [details] [review] Bug 34188: (follow-up) Rename pref to 'ForceLibrarySelection' and tidy Created attachment 158408 [details] [review] k Created attachment 158409 [details] [review] Bug 34188: Require Library Branch Selection when Logging in This patch adds the ability to force staff to select a library when logging into the staff interface. This is done via a new system preference: 'ForcedLibrarySelection' To test: 1) Apply patch, restart_all, and updatedatabase 2) Log out of the staff interface. Notice the login form looks the same and the "Library:" dropdown has 'My Library" selected as default. Log back into the staff interface. 3) In Administration, search for the system preference "ForcedLibrarySelection". Set it to 'Force' and press save. 4) Log out of the staff interface. Notice that this time, the "Library:" dropdown is required and has a blank selection as the default. 5) Fill in the username and password but do not select a library. Click 'Log in' 6) Notice you cannot log in and are asked to 'Please select an item in the list' 7) Select a library from the drop down and click 'Log in' 8) Notice the login was successful and you were logged in to the library you selected. 9) Try loggin in with some other libraries to verify it works as expected 10) Sign-off :) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158410 [details] [review] Bug 34188: (follow-up) Rename pref to 'ForceLibrarySelection' and tidy Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158411 [details] [review] Bug 34188: (QA follow-up) Fix different versions of system preference name: Force(d)LibrarySelection There were references to ForceLibrarySelection and ForceLibrarySelection (notice the additional d) in the patch. Standardized to: ForceLibrarySelection Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158412 [details] [review] Bug 34188: (QA follow-up) Add standard validation to login form This changes how the required appears. Now it will display 'our' text for requires fields instead of using the browser default. Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x |