[Bug 10277] New: Add C4::Context->IsSuperLibrarian()
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Bug ID: 10277 Summary: Add C4::Context->IsSuperLibrarian() Classification: Unclassified Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18206 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18206&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10278 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle.m.hall@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |gmcharlt@gmail.com --- Comment #2 from Galen Charlton <gmcharlt@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18206|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18421 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18421&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18421|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18422 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18422&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18425 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18425&action=edit Bug 10277 - Add C4::Context->IsSuperLibrarian() - Followup - Use IsSuperLibrarian() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #6 from Kyle M Hall <kyle.m.hall@gmail.com> ---
- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #7 from Kyle M Hall <kyle.m.hall@gmail.com> --- Test Plan: 1) Apply these patches 2) Verify superlibrarian behavior remains unchanged -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18422|0 |1 is obsolete| | Attachment #18425|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18427 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18427&action=edit Squashed both patches together. There is really no need to have them separated. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18427|Squashed both patches |Bug 10277 - Add description|together. There is really |C4::Context->IsSuperLibrari |no need to have them |an() |separated. | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10263 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10276 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18427|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18518 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18518&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #10 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- (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; -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #12 from Srdjan Jankovic <srdjan@catalyst.net.nz> ---
Does it? Did you mean
my $userenv = C4::Context->userenv or carp(...); return $userenv->{flags} % 2 == 1;
Sure, sorry, not enough coffee I guess... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18518|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19229 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19229&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19229|0 |1 is obsolete| | --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20776 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20776&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #15 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I could be wrong with those, so please double check: C4/Circulation.pm - if should become unless ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20776|0 |1 is obsolete| | --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 21144 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21144&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #17 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Kyle, did you check C4/Circulation.pm please? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #19 from Julian Maurice <julian.maurice@biblibre.com> --- Setting to Failed QA for the if/unless problem. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Joel Sasse <jsasse@plumcreeklibrary.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jsasse@plumcreeklibrary.net -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21144|0 |1 is obsolete| | --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23479 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23479&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23479|0 |1 is obsolete| | --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23480 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23480&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #22 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23481 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23481&action=edit Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23481|0 |1 is obsolete| | --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23482 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23482&action=edit Bug 10277 [Followup] - Replace all remaining userenv checks with IsSuperLibrarian() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10502 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|10278 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|10263 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|10502 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #24 from Jonathan Druart <jonathan.druart@biblibre.com> --- unit tests should be provided for the new IsSuperLibrarian routine. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23480|0 |1 is obsolete| | Attachment #23482|0 |1 is obsolete| | --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23518 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23518&action=edit Squashed the two patches together. No reason to keep them separate. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23518|Squashed the two patches |Bug 10277 - Add description|together. No reason to keep |C4::Context->IsSuperLibrari |them separate. |an() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Joel Sasse <jsasse@plumcreeklibrary.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 sandboxes@biblibre.com <sandboxes@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sandboxes@biblibre.com --- Comment #27 from sandboxes@biblibre.com <sandboxes@biblibre.com> --- Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 sandboxes@biblibre.com <sandboxes@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23518|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #28 from sandboxes@biblibre.com <sandboxes@biblibre.com> --- Created attachment 23523 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23523&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #29 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 23634 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23634&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #30 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23634|0 |1 is obsolete| | --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23635 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23635&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #32 from 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23523|0 |1 is obsolete| | Attachment #23635|0 |1 is obsolete| | --- Comment #33 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 23860 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23860&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 23861 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23861&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=7002 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 --- Comment #35 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Kyle, I found bug 7002 - it looks like there could be some more occurences to fix. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #36 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master. Thanks, Kyle and Jonathan! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |7002 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11587 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10277 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolyn.somers@biblibre.co | |m --- Comment #37 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org