The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not.
Created attachment 18206 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not.
I agree with the notion that the method for checking whether a session has superlibrarian privileges should be more clear. The patch is problematic, however: - I'm pretty sure terminating (via confess) is not the right thing to do if the userenv isn't set up yet; in particular, this has the potential to break command-line scripts for no good reason. - Doing it via confess() without loading the Carp module is wrong regardless. - A patch that introduces a new C4::Context method but never uses it is untestable. It should, at the very least, attempt to replace some of the opaque userenv flags tests, and ideally should aim to replace them all. In other words, if we're going to refactor this, let's refactor this all the way.
Created attachment 18421 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not.
Created attachment 18422 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not.
Created attachment 18425 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() - Followup - Use IsSuperLibrarian()
> - I'm pretty sure terminating (via confess) is not the right thing to do if > the userenv isn't set up yet; in particular, this has the potential to break > command-line scripts for no good reason. Good point, switched the confess to a carp! > - Doing it via confess() without loading the Carp module is wrong regardless. I've added a "use Carp" to C4::Context. I think this is the best way to do it, but the BEGIN block does some stuff with CGI::Carp. I don't believe it will cause problems, but I just wanted to point this out. > - A patch that introduces a new C4::Context method but never uses it is > untestable. It should, at the very least, attempt to replace some of the > opaque userenv flags tests, and ideally should aim to replace them all. In > other words, if we're going to refactor this, let's refactor this all the > way. I totally agree! I had originally split this off from another patch that uses it, but I'm in total agreement. I've attached a followup that switches all known code for superlibrarian checks to use IsSuperLibrarian.
Test Plan: 1) Apply these patches 2) Verify superlibrarian behavior remains unchanged
Created attachment 18427 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian()
Created attachment 18518 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
I believe it would be slightly better to write my $userenv = C4::Context->userenv or carp(...); return C4::Context->userenv->{flags} % 2 == 1; It saves one C4::Context->userenv call. I admire this kind of work, having C4::Context->userenv->{flags} % 2 != 1 all over the place is just scandalous.
(In reply to comment #10) > my $userenv = C4::Context->userenv or carp(...); > return C4::Context->userenv->{flags} % 2 == 1; > > It saves one C4::Context->userenv call. Does it? Did you mean my $userenv = C4::Context->userenv or carp(...); return $userenv->{flags} % 2 == 1;
> Does it? Did you mean > > my $userenv = C4::Context->userenv or carp(...); > return $userenv->{flags} % 2 == 1; Sure, sorry, not enough coffee I guess...
Created attachment 19229 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
Created attachment 20776 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
I could be wrong with those, so please double check: C4/Circulation.pm - if should become unless ?
Created attachment 21144 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
Kyle, did you check C4/Circulation.pm please?
Hi, I just wanted to report that it remains files where IsSuperLibrarian could be used: $ git grep '{flags}' | grep 1 C4/Acquisition.pm: if ( ($userenv) && ( $userenv->{flags} != 1 ) ) { C4/Acquisition.pm: if ( ($userenv) && ( $userenv->{flags} != 1 ) ) { C4/Acquisition.pm: && C4::Context->userenv->{flags} != 1 ) { C4/Acquisition.pm: if ( $userenv && ($userenv->{flags} || 0) != 1 ) { C4/Branch.pm: C4::Context->userenv->{flags} %2 != 1 && C4/Context.pm: return $userenv->{flags} % 2 == 1; C4/Items.pm: elsif ( !( C4::Context->userenv->{flags} & 1 ) acqui/basket.pl: unless ( $userenv->{flags} == 1 ) { acqui/basket.pl: unless ( $userenv->{flags} == 1 ) { acqui/neworderempty.pl: C4::Context->userenv->{flags}!=1 && cataloguing/additem.pl: C4::Context->userenv->{flags}!=1 && circ/ysearch.pl: && (C4::Context->userenv->{flags} % 2) !=1 suggestion/suggestion.pl: C4::Context->userenv->{flags}!=1 && tools/export.pl: && !( C4::Context->userenv->{flags} & 1 ) tools/export.pl: && !( C4::Context->userenv->{flags} & 1 ) ) and I think Srdjan is right: 'if' should become 'unless' in C4/Circulation.pm
Setting to Failed QA for the if/unless problem.
Created attachment 23479 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
Created attachment 23480 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged
Created attachment 23481 [details] [review] Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian()
Created attachment 23482 [details] [review] Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian()
unit tests should be provided for the new IsSuperLibrarian routine.
Created attachment 23518 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian()
(In reply to Jonathan Druart from comment #24) > unit tests should be provided for the new IsSuperLibrarian routine. I have been unable to find a good way to UT this sub. MockModule doesn't seem to work in this context, since we are trying to mock a subroutine outside the module and then use the mocked sub inside the module. On the other hand, this subroutine is literally two lines. If you have an idea on how to unit test this sub, please let me know!
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Created attachment 23523 [details] [review] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged Signed-off-by: Joel Sasse <jsasse@plumcreeklibrary.net>
Created attachment 23634 [details] [review] Bug 10277: Add UT for C4::Context::IsSuperLibrarian Note that I modify the return value. Before this patch, it returned an empty string or 1. Now it returns 0 or 1. Test plan: - same as the original patch - verify that unit tests pass: prove t/Context.t
(In reply to Kyle M Hall from comment #26) > I have been unable to find a good way to UT this sub. MockModule doesn't > seem to work in this context, since we are trying to mock a subroutine > outside the module and then use the mocked sub inside the module. It seems to work using MockModule. Have a look at my patch.
Created attachment 23635 [details] [review] Bug 10277: Add UT for C4::Context::IsSuperLibrarian Note that I modify the return value. Before this patch, it returned an empty string or 1. Now it returns 0 or 1. Test plan: - same as the original patch - verify that unit tests pass: prove t/Context.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Great work! Thanks! I had attempted a UT using MockModule, but I must have been setting it up incorrectly. (In reply to Jonathan Druart from comment #30) > (In reply to Kyle M Hall from comment #26) > > I have been unable to find a good way to UT this sub. MockModule doesn't > > seem to work in this context, since we are trying to mock a subroutine > > outside the module and then use the mocked sub inside the module. > > It seems to work using MockModule. Have a look at my patch.
Created attachment 23860 [details] [review] [PASSED QA] Bug 10277 - Add C4::Context->IsSuperLibrarian() The method of checking the logged in user for superlibrarian privileges is obtuse ( $userenv && $userenv->{flags} % 2 != 1 ) to say the least. The codebase is littered with these lines, with no explanation given. It would be much better if we had one subroutine that returned a boolean value to tell us if the logged in user is a superlibrarian or not. Test Plan: 1) Apply this patch 2) Verify superlibrarian behavior remains unchanged Signed-off-by: Joel Sasse <jsasse@plumcreeklibrary.net> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Comments on second patch.
Created attachment 23861 [details] [review] [PASSED QA] Bug 10277: Add UT for C4::Context::IsSuperLibrarian Note that I modify the return value. Before this patch, it returned an empty string or 1. Now it returns 0 or 1. Test plan: - same as the original patch - verify that unit tests pass: prove t/Context.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script, including new tests. Checked the code and tested superlibrarian behaviour in some places: moremember.pl: With IndyBranches only superlibrarian can delete borrowers from other branches. Accessing the borrower with a direct link. OK C4/Members.pm With IndyBranches only superlibrarian can search for borrowres from other branches. OK tools/holidays.pl With IndyBranches only superlibrarian can edit holidays for other branches.
Hi Kyle, I found bug 7002 - it looks like there could be some more occurences to fix.
Pushed to master. Thanks, Kyle and Jonathan!
Pushed to 3.14.x, will be in 3.14.10 This enhancement applies perfectly to 3.14.x and is use in many patches so I decided in the end to integrate it.