Bug 10277 - Add C4::Context->IsSuperLibrarian()
Summary: Add C4::Context->IsSuperLibrarian()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 7002 10276 11587
  Show dependency treegraph
 
Reported: 2013-05-17 15:01 UTC by Kyle M Hall
Modified: 2015-06-04 23:30 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10277 - Add C4::Context->IsSuperLibrarian() (1.09 KB, patch)
2013-05-17 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (1.44 KB, patch)
2013-05-28 12:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (1.24 KB, patch)
2013-05-28 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() - Followup - Use IsSuperLibrarian() (16.13 KB, patch)
2013-05-28 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (17.15 KB, patch)
2013-05-28 12:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (17.14 KB, patch)
2013-05-30 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (17.11 KB, patch)
2013-06-25 11:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (17.03 KB, patch)
2013-09-04 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (17.03 KB, patch)
2013-09-17 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (16.50 KB, patch)
2013-12-12 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (16.50 KB, patch)
2013-12-12 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian() (8.54 KB, patch)
2013-12-12 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian() (8.78 KB, patch)
2013-12-12 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (24.85 KB, patch)
2013-12-13 15:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10277 - Add C4::Context->IsSuperLibrarian() (24.91 KB, patch)
2013-12-13 16:46 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 10277: Add UT for C4::Context::IsSuperLibrarian (2.20 KB, patch)
2013-12-18 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10277: Add UT for C4::Context::IsSuperLibrarian (2.25 KB, patch)
2013-12-18 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10277 - Add C4::Context->IsSuperLibrarian() (24.96 KB, patch)
2013-12-29 10:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10277: Add UT for C4::Context::IsSuperLibrarian (2.77 KB, patch)
2013-12-29 10:14 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-05-17 15:01:30 UTC
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.
Comment 1 Kyle M Hall 2013-05-17 15:02:02 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-05-20 19:48:30 UTC
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.
Comment 3 Kyle M Hall 2013-05-28 12:08:42 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-05-28 12:10:53 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-05-28 12:38:38 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-05-28 12:41:51 UTC
> - 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.
Comment 7 Kyle M Hall 2013-05-28 12:42:40 UTC
Test Plan:
1) Apply these patches
2) Verify superlibrarian behavior remains unchanged
Comment 8 Kyle M Hall 2013-05-28 12:46:30 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-05-30 14:05:53 UTC Comment hidden (obsolete)
Comment 10 Srdjan Jankovic 2013-06-03 04:50:46 UTC
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.
Comment 11 Galen Charlton 2013-06-03 16:09:11 UTC
(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;
Comment 12 Srdjan Jankovic 2013-06-04 00:07:04 UTC
> Does it?  Did you mean
> 
> my $userenv = C4::Context->userenv or carp(...);
> return $userenv->{flags} % 2 == 1;

Sure, sorry, not enough coffee I guess...
Comment 13 Kyle M Hall 2013-06-25 11:38:57 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-09-04 11:17:10 UTC Comment hidden (obsolete)
Comment 15 Srdjan Jankovic 2013-09-05 05:27:21 UTC
I could be wrong with those, so please double check: C4/Circulation.pm - if should become unless ?
Comment 16 Kyle M Hall 2013-09-17 16:57:32 UTC Comment hidden (obsolete)
Comment 17 Srdjan Jankovic 2013-09-18 22:59:01 UTC
Kyle, did you check C4/Circulation.pm please?
Comment 18 Julian Maurice 2013-09-24 10:06:06 UTC
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
Comment 19 Julian Maurice 2013-09-24 10:07:14 UTC
Setting to Failed QA for the if/unless problem.
Comment 20 Kyle M Hall 2013-12-12 13:03:51 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-12-12 13:20:52 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-12-12 13:21:00 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-12-12 13:31:03 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2013-12-13 12:15:05 UTC
unit tests should be provided for the new IsSuperLibrarian routine.
Comment 25 Kyle M Hall 2013-12-13 15:03:13 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-12-13 15:29:01 UTC
(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!
Comment 27 Biblibre Sandboxes 2013-12-13 16:45:32 UTC
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 28 Biblibre Sandboxes 2013-12-13 16:46:29 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-12-18 15:14:55 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2013-12-18 15:16:11 UTC
(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.
Comment 31 Kyle M Hall 2013-12-18 15:29:43 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2013-12-18 15:31:20 UTC
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.
Comment 33 Katrin Fischer 2013-12-29 10:13:32 UTC
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.
Comment 34 Katrin Fischer 2013-12-29 10:14:03 UTC
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.
Comment 35 Katrin Fischer 2013-12-29 15:41:41 UTC
Hi Kyle, I found bug 7002 - it looks like there could be some more occurences to fix.
Comment 36 Galen Charlton 2013-12-30 15:51:53 UTC
Pushed to master.  Thanks, Kyle and Jonathan!
Comment 37 Fridolin Somers 2014-08-07 07:46:57 UTC
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.