Bug 26383 - Koha::Patron->is_superlibrarian is not optimal
Summary: Koha::Patron->is_superlibrarian is not optimal
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 23634
Blocks:
  Show dependency treegraph
 
Reported: 2020-09-04 09:44 UTC by Jonathan Druart
Modified: 2024-07-15 18:19 UTC (History)
1 user (show)

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


Attachments
Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian (11.85 KB, patch)
2021-06-23 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-09-04 09:44:09 UTC
It must not call $self->has_permission which does a DB request. C4::Context::IsSuperLibrarian is what must be done: $self->flags % 2

It could be good to replace the occurrence of IsSuperLibrarian at the same time, it's a bad pattern to have twice the same method to check for this superpower.
Comment 1 Jonathan Druart 2021-06-23 14:27:20 UTC
Just had another look at this one. The problem is that we are calling it from modules, and have to Koha::Patrons->find($userenv->{number}) when, before, we had just to return $flags % 2
Comment 2 Jonathan Druart 2021-06-23 14:29:59 UTC
Created attachment 122341 [details] [review]
Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian
Comment 3 Jonathan Druart 2021-06-23 14:31:20 UTC
I am not convinced here, we may want to replace it from .pl but keep C4::Context->IsSuperLibrarian for modules. Then we are not able to remove it and still use the code in two places.

Any suggestions?
Comment 4 David Cook 2021-06-23 23:37:30 UTC
Comment on attachment 122341 [details] [review]
Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian

Review of attachment 122341 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +1073,5 @@
> +
> +    my $userenv = C4::Context->userenv;
> +    if ( C4::Context->preference("IndependentBranches") && $userenv ) {
> +        my $logged_in_user = Koha::Patrons->find($userenv->{number});
> +        unless ( $logged_in_user->is_superlibrarian ) {

Technically, the session stores the flags value, so we could just do the calculation off the session data. The only downside of doing that is that we probably only set the flags session data at login time, so if someone's permissions change while they're logged in, it won't be detected until they've logged out and logged back in. But... it would mean fewer database calls.

If we're doing a multi-checkout, the number of database calls would increase by that factor...
Comment 5 David Cook 2021-06-23 23:39:03 UTC
Comment on attachment 122341 [details] [review]
Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian

Review of attachment 122341 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +1073,5 @@
> +
> +    my $userenv = C4::Context->userenv;
> +    if ( C4::Context->preference("IndependentBranches") && $userenv ) {
> +        my $logged_in_user = Koha::Patrons->find($userenv->{number});
> +        unless ( $logged_in_user->is_superlibrarian ) {

I suppose a more optimal design would be to pass the issuer to CanBookBeIssued via a parameter, but that would probably be too dramatic a refactor for us to do at this point...
Comment 6 David Cook 2021-06-23 23:50:16 UTC
(In reply to Jonathan Druart from comment #3)
> I am not convinced here, we may want to replace it from .pl but keep
> C4::Context->IsSuperLibrarian for modules. Then we are not able to remove it
> and still use the code in two places.
> 
> Any suggestions?

Another option could be something like this:

my $is_superlibrarian = Koha::Auth->is_superlibrarian({
    borrowernumber => $borrowernumber,
});
my $is_superlibrarian = Koha::Auth->is_superlibrarian({
    patron => $patron,
});

The only downside of that would be needing to add "use Koha::Auth", although I suppose we could add that into C4::Auth, which is called in the .pl scripts because they need the "get_template_and_user" function. 

Alternatively, you could write Koha::Patron->is_superlibrarian to be used as either a class method or an object method. You just check whether $self is a string or a reference to an object. If it's a class method (ie $self is a string), you could require a "patron" or "borrowernumber" parameter. 

That way you're keeping the code in one place but you're allowing for 2 different ways of calling it.

There's some ideas :D