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.
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
Created attachment 122341 [details] [review] Bug 26383: WIP Replace C4::Context->IsSuperLibrarian with Koha::Patron->is_superlibrarian
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 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 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...
(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